search

json-schema-org / JSON-Schema-Test-Suite

A language agnostic test suite for the JSON Schema specifications
MIT License
603 stars 205 forks source link

Adding CI workflow for annotations of specification property #742

Closed Era-cell closed 2 months ago

Era-cell commented 2 months ago

Adressing this issue: https://github.com/json-schema-org/JSON-Schema-Test-Suite/issues/728 This work: I have some queries:

  1. Its printing in the run python script. Need help in printing in Annotations section
  2. I dont know whther the ::notice keyword suites this more
  3. for draft3 - The spec is a pdf, so do I have have to write exceptional case for it. I guess we can keep another property in front of ::notice
  4. And check on the folder structure needed
karenetheridge commented 2 months ago

There's a lot of correction commits in here that should be squashed together.

Era-cell commented 2 months ago

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...😕

Era-cell commented 2 months ago

Hi.... should I reopen the PR?? or can tell me if this method is wrong too. I am ready for any changes...

Julian commented 2 months ago

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 gitpython, which I wouldn't expect you to be doing -- you should rely on the workflow to check out the current version of the repo but then you should simply be able to interact with the filesystem (i.e. use Path.walk probably) to iterate over all the JSON files in the tests/ directory.

Julian commented 2 months ago

for draft3 - The spec is a pdf, so do I have have to write exceptional case for it. I guess we can keep another property in front of ::notice

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.

Julian commented 2 months ago

And sorry for the triple post but as for

Its printing in the run python script. Need help in printing in Annotations section

You need to redirect your output when you run your python script.py thing, to >>$GITHUB_OUTPUT I think but I forget, check the docs I linked above.

Era-cell commented 2 months ago

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 gitpython, which I wouldn't expect you to be doing -- you should rely on the workflow to check out the current version of the repo but then you should simply be able to interact with the filesystem (i.e. use Path.walk probably) to iterate over all the JSON files in the tests/ directory.

I felt the need of pygithub to identify new files and lines in the present PR, Is it that by writing script to walk over all the files in /tests* automatically only those files or lines changed will run the script we write to print annotations. If not, then by annotating all specifications it will be 100s of annotations.. so, I understand that we cannot rely on pygithub as its a library and better to rely on shell scripts, can I get an example of some annotations anywhere which does similar task.

Julian commented 2 months ago

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.

If not, then by annotating all specifications it will be 100s of annotations

(There's nothing inherently wrong with this).

I understand that we cannot rely on pygithub as its a library and better to rely on shell scripts, can I get an example of some annotations anywhere which does similar task.

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 :)

Era-cell commented 2 months ago

Hi, @Julian I have made the changes, now it annotated without using pygithub, It also creates a section unchanged files with annotations during all the PRs.. is that good like this Screenshot 2024-04-30 123105

Julian commented 2 months ago

It also creates a section unchanged files with annotations during all the PRs.. is that good like this

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.

Era-cell commented 2 months ago

All done @Julian, also added line number by taking the description of the testcase ig if that's ok. Can you review my PR Screenshot 2024-05-01 022756 this is an Example of the annotations collection Screenshot 2024-05-01 024442

Era-cell commented 2 months ago

@Julian I have done the changes...

Julian commented 2 months ago

@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 Path.read_text).

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!

Era-cell commented 2 months ago

@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 Path.read_text).

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 ❤️