Adding CI workflow for annotations of specification property #742
Conversation
|
There's a lot of correction commits in here that should be squashed together. |
I have squashed it into First Workflow2. I don't know if I have did it properly...😕 |
|
Hi.... should I reopen the PR?? or can tell me if this method is wrong too. I am ready for any changes... |
|
I'm not sure about what we do in other repositories in the org, but we don't have any such policy in this repo to ask for squashed commits from newer contributors so as far as I'm concerned personally you're fine. I haven't looked yet in detail at what to suggest but what I saw as the more important thing is you're still interacting with |
That's odd, I could have sworn it didn't used to be but maybe I'm wrong. Anyways, it's almost certainly fine even if we leave draft 3 out entirely, we write in the readme that support for it here is frozen so certainly not worth your time I think. |
|
And sorry for the triple post but as for
You need to redirect your output when you run your |
I felt the need of pygithub to identify new files and lines in the present PR, |
What I was trying to mention before is I have to believe that functionality is going to be natively supported in GitHub Actions somehow, but as I said previously I know I'm not giving you where precisely (as I haven't researched it myself). But IIRC I pointed you at a place to look (and if not I can give it another few minutes to find it). I think I mentioned I'd try just outputting all of them, and seeing whether the GitHub UI then simply does the right thing and only shows the relevant ones for the PR.
(There's nothing inherently wrong with this).
No, shell scripts are definitely not what we want to rely on, I simply am saying I think your script can be a third of the size if you rely on the above :) |
|
Hi, @Julian I have made the changes, now it annotated without using pygithub, |
That seems fine/good to me. Can you share what it looks like in the "real" case? (I.e. for lines that do have changes)? I'll have to leave you some comments on your script. |
| @@ -0,0 +1,23 @@ | |||
| name: Specifications Annotation Workflow | |||
There was a problem hiding this comment.
| name: Specifications Annotation Workflow | |
| name: Show Specification Annotations |
And maybe rename the file to show_specification_annotations.yml.
| - 'tests/**' | ||
|
|
||
| jobs: | ||
| run_tests: |
There was a problem hiding this comment.
| run_tests: | |
| annotate: |
|
|
||
| jobs: | ||
| run_tests: | ||
| name: logging annotations |
There was a problem hiding this comment.
I'd just remove this, the workflow name is/will be fine.
|
|
||
| steps: | ||
| - name: Checkout Repository | ||
| uses: actions/checkout@v3 |
There was a problem hiding this comment.
| uses: actions/checkout@v3 | |
| uses: actions/checkout@v4 |
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - name: Checkout Repository |
There was a problem hiding this comment.
No need for the name here I think.
bin/annotation_workflow.py
Outdated
| print(f"::warning file={file},line=1::Annotation: {url}") | ||
|
|
||
| # Read specification URLs from JSON file | ||
| with open("bin/specification_urls.json", "r") as f: |
There was a problem hiding this comment.
Use pathlib.Path here and below instead of open or the os module.
bin/annotation_workflow.py
Outdated
| # Iterate through JSON files in tests folder and subdirectories | ||
| for root, dirs, files in os.walk("tests"): | ||
| for file_name in files: | ||
| if file_name.endswith(".json"): |
There was a problem hiding this comment.
Here you'll use Path.rglob(".json") instead of .walk.
bin/annotation_workflow.py
Outdated
| elif spec in ["ecma262", "perl5"]: | ||
| print_github_action_notice(file_path, urls[spec] + section) | ||
| elif re.match("^rfc\\d+$", spec): | ||
| print_github_action_notice(file_path, urls["rfc"] + spec + ".txt#" + section) |
There was a problem hiding this comment.
This is a bit obtuse. Can you instead take a quick look into whether it's easy to use a URL template (which will likely mean adding a dependency on a Python URL template library), which will allow you to instead put this logic in specification_urls.json rather than special casing it here.
bin/annotation_workflow.py
Outdated
| else: | ||
| print_github_action_notice(file_path, urls["iso"]) | ||
| except json.JSONDecodeError: | ||
| print(f"Failed to parse JSON content for file: {file_path}") |
There was a problem hiding this comment.
Your print I suspect will go nowhere here so no one will see it, even though other CI will fail anyhow. But ideally you need to instead emit an error message to GitHub actions, and probably also be exiting with non-zero status.
bin/specification_urls.json
Outdated
| "perl5": "https://perldoc.perl.org/perlre#", | ||
| "rfc": "https://www.rfc-editor.org/rfc/", | ||
| "iso": "https://www.iso.org/obp/ui" | ||
| } |
There was a problem hiding this comment.
Please configure your editor to make sure all files end in a newline or be sure you do so manually, one is missing here.
|
All done @Julian, also added line number by taking the description of the testcase ig if that's ok. |
bin/annotation_workflow.py
Outdated
| Print GitHub action notice with file path, URL, and line number. | ||
|
|
||
| Parameters: | ||
| file (str): File path. |
There was a problem hiding this comment.
Good you added these! Though in modern Python they go in the signature, so here:
github_action_notice(file: str, url: str, line: int)
(though probably file should be a Path)
bin/annotation_workflow.py
Outdated
|
|
||
| def github_action_notice(file, url, line): | ||
| """ | ||
| Print GitHub action notice with file path, URL, and line number. |
There was a problem hiding this comment.
| Print GitHub action notice with file path, URL, and line number. | |
| Return a GitHub action notice with file path, URL, and line number. |
bin/annotation_workflow.py
Outdated
| json_file_path = Path("bin/specification_urls.json") | ||
|
|
||
| # Read specification URLs from JSON file using pathlib.Path | ||
| with json_file_path.open("r", encoding="utf-8") as f: |
There was a problem hiding this comment.
| with json_file_path.open("r", encoding="utf-8") as f: | |
| urls = json.loads(json_file_path.read_text()) |
bin/annotation_workflow.py
Outdated
| else: | ||
| url = urls[spec].format(spec=spec, section=section) | ||
|
|
||
| clear_previous_annotations() |
There was a problem hiding this comment.
Is this really correct? Why do we clear all annotations each time?
There was a problem hiding this comment.
The annotations were getting stacked up in the same place whenever commits were made on the same PR, so I found this solution
There was a problem hiding this comment.
But this line clears them each test -- if that's the case, shouldn't we be clearing them simply once at the top of the run?
There was a problem hiding this comment.
Let me test it...
There was a problem hiding this comment.
It works even specified at the top. 👍
|
@Julian I have done the changes... |
|
@Era-cell you had a few comments of mine that weren't addressed (fixing/adding type annotations instead of docstrings, fixing the relative paths, using I made those changes along with a few additional ones (adding docstrings in a place or two, along with slightly improved line information) in #746 which is on top of your commits. Going to close this, I'll merge your changes in that other PR after reverting the change to additionalProperties.json and then we can see how this goes. Appreciate your work, well done! |
Okay.. I'll note it. I have got a lot to learn from here. Thank you ❤️ |
Adressing this issue: #728
This work: I have some queries: