-
Notifications
You must be signed in to change notification settings - Fork 36
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
Add a script to automatically update the README.md links #212
Conversation
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.
Looks good. Will people remember to use it? We'll see...
Can we not change edm4hep.yaml to remove those spaces? With the changes I propose that is not needed. They will make git blame a bit less useful since many lines will point to this commit and you can easily fix it by pasting the current version of edm4hep.yaml.
If we want the changes it's fine but then let them be in a different commit that we can add to gitignore revs.
Very nice. Thanks for picking this up (that was not my intention with the comment in #211). How hard would it be to extract a function that simply checks whether the lines in the yaml match with the links in the README? That would be rather easy to hook up to CI, I think and then at least we would have a check so that people would know to run the script. Style(?) comment: We could use f-string formatting since this will only ever be run with python3, but that is more of a personal preference. |
For the whitespace changes in the yaml file, I would say we change the ones that are directly in the yaml structure, e.g. these, but we do not touch the ones that are in |
Would this work for the CI? |
$ pre-commit run -a readme-links-check
README links check.......................................................Failed
- hook id: readme-links-check
- exit code: 1
Vector3f is wrongly linked (line 23 --> 11)
README.md links should be updated. (Use the scripts/updateReadmeLinks.py script) This seems to work as intended if I artificially change a line number. |
And also runs in CI: https://github.com/key4hep/EDM4hep/actions/runs/5486162834/jobs/9995897962?pr=212#step:4:873 @jmcarcell do you have anything to add? |
Not much, other than if this is squashed then the commits that change whitespaces and the changes will be in the same commit. It's actually well separated in two commits so removing those 2 should be use and they can go in another PR |
I can also make everything into two commits, keeping the formatting in one and the rest int he other, and then we do a "rebase and merge" |
- Update README.md links - Add a pre-commit hook to check the README links
Should be done now. |
Thanks again @BrieucF for putting this script together. |
BEGINRELEASENOTES
ENDRELEASENOTES