refactor: derive bundle runtime paths from source_dir - #820
AlexanderLanin wants to merge 1 commit into
Conversation
AlexanderLanin
changed the title
There was a problem hiding this comment.
🟢 Approval recommended
The focused refactor preserves existing explicit-source behavior and is covered by relevant package-root, nested-package, and cross-module scenarios.
Pull request overview
Refactors bundle path derivation to retain source_dir until rule evaluation.
Changes:
- Pass
source_dirdirectly into bundle creation. - Derive local and external runtime paths from the bundle label.
File summaries
| File | Description |
|---|---|
docs.bzl |
Forwards and documents source_dir. |
bzl/bundle_rules.bzl |
Derives runtime paths within the bundle rule. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Documentation preview for this pull request is available at: |
| @@ -453,7 +456,7 @@ def create_bundle( | |||
| source_dir_globbed = source_dir_globbed, | |||
| source_targets = source_targets, | |||
| sourcelinks_json = sourcelinks_json, | |||
| strip_prefix = strip_prefix, | |||
| source_dir = source_dir if source_dir != None else "", | |||
There was a problem hiding this comment.
But since you put source_dir in default above as "" would the source_dir if in 459 not always be true then, caues it's never the default None?
There was a problem hiding this comment.
when a user calls docs or docs_bundle the source_dir defaults to None. This place is exactly where None is transformed to "". Maybe it makes more sense to store None instead of empty strings, but that would be the next refactoring starting from this point.
There was a problem hiding this comment.
I mean here then you say the attribute is `""`` per default.
Is this not an issue?
https://github.com/eclipse-score/docs-as-code/pull/820/changes#diff-637f1bfda16cc7049dbae85157101729a0cadc42705e2acce9653e9382e9c096R425-R427
There was a problem hiding this comment.
That default "" is exactly what is being set here
32286b5 to
124b2ed
Compare
124b2ed to
b258efa
Compare
|
FYI stacked PRs can apparently only be force pushed, not merged with main |
| "source_dir_globbed": attr.label_list(allow_files = True), | ||
| "source_targets": attr.label_list(allow_files = True), | ||
| "sourcelinks_json": attr.label(allow_single_file = True), | ||
| "strip_prefix": attr.string(default = ""), |
There was a problem hiding this comment.
Strip_prefix was a missnamed argument. It never actually stripped a prefix.
That's why now it has been removed and actuall source_dir has been added.
Yes once ,one is approved it will get |
What this achieves
Passes the original
source_dirinto bundle creation so the internal bundle rule can derive the source runtime path from the owning package and repository.Why this is needed
The current bundle setup passes a precomputed
strip_prefixand infers external-repository paths from the first globbed file. Keeping the source-directory value until the rule boundary makes path ownership explicit and handles package-root and nested source directories consistently.Changes
strip_prefixattribute and argument withsource_dir.source_dir.