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

prevent absolute links to docs (except badge) #238

Merged
merged 6 commits into from
Oct 3, 2024

Conversation

BalzaniEdoardo
Copy link
Collaborator

@BalzaniEdoardo BalzaniEdoardo commented Oct 3, 2024

Added automation to the CI that checks for no absolute links to NeMoS docs.

The problem that absolute links may cause is described here #226.

Quoting

All links to NeMoS docs pages must be relative. If absolute links to docs pages are added, structural changes in the HTML may break the absolute links, but this break won't be detected the first time the CI is run (this is because the htmlproofer would check the absolute link that will point to an older version of the website - "https://nemos.readthedocs.io/stable" or ".../latest" -, instead of the current built).

Note

The CONTRIBUTING.md contains a note that clarifies to use only relative links already. Look at the end of the file.

@codecov-commenter
Copy link

codecov-commenter commented Oct 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.24%. Comparing base (32132b1) to head (7b07727).
Report is 429 commits behind head on development.

Additional details and impacted files
@@               Coverage Diff               @@
##           development     #238      +/-   ##
===============================================
- Coverage        97.30%   97.24%   -0.06%     
===============================================
  Files               18       20       +2     
  Lines             1669     1815     +146     
===============================================
+ Hits              1624     1765     +141     
- Misses              45       50       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@billbrod billbrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine.

  • Note that if you run chmod +x prevent_absolute_links_to_docs.sh locally and then commit those changes, you won't need to run it in the CI (git tracks permissions as well).
  • I would move this script from the root directory. Maybe to a new folder called scripts/ or include it in the .github directory? so as not to clutter up the root (I'm pretty sure that you'll have to change the path it's called on then).
  • maybe replace grep -r nemos.readthedocs.io with grep -r -E https?://nemos.*, since we're going to change the domain name soon. (the -E allows you to use extended regex (not sure how that differs from regular regex, it's what I use), and s? means it will match http://nemos or https://nemos`).
  • I'd probably add an explanation in CONTRIBUTING as to what relative vs. absolute means. I'm not sure it's obvious to the average scientist. E.g., "links must be relative, not absolute. That is, they should be of the form ./path/to/page rather than https://nemos.readthedocs.io/path/to/page." or something like that

@BalzaniEdoardo
Copy link
Collaborator Author

I think this is fine.

* Note that if you run `chmod +x prevent_absolute_links_to_docs.sh` locally and then commit those changes, you won't need to run it in the CI (git tracks permissions as well).

that's cool

* I would move this script from the root directory. Maybe to a new folder called `scripts/` or include it in the `.github` directory? so as not to clutter up the root (I'm pretty sure that you'll have to change the path it's called on then).

ok!

* maybe replace `grep -r nemos.readthedocs.io` with `grep -r -E https?://nemos.*`, since we're going to change the domain name soon. (the `-E` allows you to use extended regex (not sure how that differs from regular regex, it's what I use), and `s?` means it will match `http://nemos` or [https://nemos`](https://nemos%60)).

nice regex trick

* I'd probably add an explanation in `CONTRIBUTING` as to what relative vs. absolute means. I'm not sure it's obvious to the average scientist. E.g., "links must be relative, not absolute. That is, they should be of the form `./path/to/page` rather than `https://nemos.readthedocs.io/path/to/page`." or something like that

will do

@BalzaniEdoardo
Copy link
Collaborator Author

After a second thought, I think it is safer to keep the explicit permission setting in the ci, and the execution time is basically minimal

@BalzaniEdoardo BalzaniEdoardo merged commit 57ed379 into development Oct 3, 2024
12 checks passed
@BalzaniEdoardo BalzaniEdoardo deleted the ci_absolute_link_to_docs branch October 3, 2024 18:53
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