-
Notifications
You must be signed in to change notification settings - Fork 20
Source links as Bazel target #358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run //src:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
| import sys | ||
| from pathlib import Path | ||
|
|
||
| from src.extensions.score_source_code_linker.generate_source_code_links_json import ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might need to either make this function public or make a public counterpart.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure yet. Eventually, we should be able to extract it from the Sphinx extension, because the extension shall only consume the json files.
MaximilianSoerenPollak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great from my side.
Script part is simple and concise.
Just had some questions regarding some of the things done as I don't understand them.
d781b67 to
529deb8
Compare
MaximilianSoerenPollak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall great stuff, thanks for the work.
Just couple comments / questions.
| In you ``BUILD`` files, you specify which source files to scan | ||
| with ``filegroup`` or ``glob`` or whatever Bazel mechanism you prefer. | ||
| Then, you use the ``sourcelinks_json`` rule to scan those files. | ||
| Finally, pass the scan results to the ``docs`` rule as ``sourcelinks`` attribute. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remember correctly it was requested that we use non direct language (like you etc.)
Though I don't care too much about it personally.
What's the opinion of the others?
Here is a suggestion how that could look like:
| In you ``BUILD`` files, you specify which source files to scan | |
| with ``filegroup`` or ``glob`` or whatever Bazel mechanism you prefer. | |
| Then, you use the ``sourcelinks_json`` rule to scan those files. | |
| Finally, pass the scan results to the ``docs`` rule as ``sourcelinks`` attribute. | |
| In the ``BUILD`` files, it can be specified which source files should be scanned. | |
| This can be achieved with ``filegroup``, ``glob`` or whatever Bazel mechanism is prefered. | |
| Then, the ``sourcelinks_json`` rule is used to scan those files. | |
| Finally, the scanned results need to be passed into ``docs`` rule as ``sourcelinks`` attribute. |
71b7a05 to
cbb7a3f
Compare
Currently, we are not using it yet, but this commit adds the Bazel scaffolding to make the json data available.
Fixes consumer checks
8eae746 to
502f550
Compare
|
| # ******************************************************************************* | ||
|
|
||
| """ | ||
| Merge multiple sourcelinks JSON files into a single JSON file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scripts directory is only for local development (linters; super confusing) so far.
This is the bazel side of src/extensions/score_source_code_linker? I would still put it to src/extensions/score_source_code_linker/bazel_prebuild_step?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add README.md files to any new directory explaining what that is and a rough sketch of the architecture?
| # ******************************************************************************* | ||
|
|
||
| """ | ||
| Merge multiple sourcelinks JSON files into a single JSON file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add README.md files to any new directory explaining what that is and a rough sketch of the architecture?
|
|
||
| merged = [] | ||
| for json_file in args.files: | ||
| if json_file.exists(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no warning on not exists?
| if json_file.exists(): | ||
| with open(json_file) as f: | ||
| data = json.load(f) | ||
| if isinstance(data, list): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no warning on else?
| srcs = glob( | ||
| [ | ||
| "*.py", | ||
| "*.yaml", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's super easy to forget some file here. Is a missing source code link safety relevant?
@RolandJentschETAS do you know?
| "tests/**/*.rst", | ||
| "tests/**/*.yaml", | ||
| ], | ||
| ] + ["tests/rst/conf.py"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ] + ["tests/rst/conf.py"], | |
| "tests/rst/conf.py"], |
Why do we need to add this file now?
| get_cache_filename(outdir, "score_source_code_linker_cache.json") | ||
| ) | ||
| source_code_links_json = os.environ.get("SCORE_SOURCELINKS") | ||
| if not source_code_links_json: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this if? Maybe in some overarching concept? Maybe in a separate PR ;-)
| "//src/extensions/score_sphinx_bundle:all_sources", | ||
| "//src/extensions/score_sync_toml:all_sources", | ||
| "//src/find_runfiles:all_sources", | ||
| "//src/helper_lib:all_sources", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No clever collection mechanism in bazel? This looks extremely error prone.
| sourcelinks_json( | ||
| name = "sourcelinks_json", | ||
| srcs = [ | ||
| "//src:all_sources", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all following items are already included in src:all_sources?
| "@score_process//:needs_json", | ||
| ], | ||
| source_dir = "docs", | ||
| sourcelinks = [":my_source_links"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
up to now docs() is defining internal targets like needs_json itself. All in one command. Can we do the same with sourcelinks_json?
📌 Description
Provides a Bazel command for source links to make source dependency explicit.
Use this target in
docs()instead of implicitly scanning the git repo.This allows to have source links even for other modules.
🚨 Impact Analysis
✅ Checklist