Skip to content
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

Merged
merged 2 commits into from
Jul 7, 2023

Conversation

BrieucF
Copy link
Contributor

@BrieucF BrieucF commented Jul 7, 2023

BEGINRELEASENOTES

  • Add a script to automatically update the README.md links

ENDRELEASENOTES

Copy link
Contributor

@jmcarcell jmcarcell left a 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.

scripts/updateReadmeLineNumbers.py Outdated Show resolved Hide resolved
scripts/updateReadmeLineNumbers.py Outdated Show resolved Hide resolved
scripts/updateReadmeLineNumbers.py Outdated Show resolved Hide resolved
@tmadlener
Copy link
Contributor

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.

@tmadlener
Copy link
Contributor

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 ExtraCode blocks.

@BrieucF
Copy link
Contributor Author

BrieucF commented Jul 7, 2023

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.

Would this work for the CI?

@tmadlener
Copy link
Contributor

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

@tmadlener
Copy link
Contributor

@jmcarcell
Copy link
Contributor

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

@tmadlener
Copy link
Contributor

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
@tmadlener
Copy link
Contributor

Should be done now.

@tmadlener tmadlener enabled auto-merge (rebase) July 7, 2023 14:30
@tmadlener tmadlener disabled auto-merge July 7, 2023 14:31
@tmadlener tmadlener merged commit 93effb8 into key4hep:master Jul 7, 2023
@tmadlener
Copy link
Contributor

Thanks again @BrieucF for putting this script together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants