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

Normalize links to other ADR documents #318

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

bannatech
Copy link
Contributor

.readme.templ Outdated
@@ -25,6 +25,6 @@ We want to move away from using these to document individual minor decisions, mo

## Template

Please see the [template](adr-template.md). The template body is a guideline. Feel free to add sections as you feel appropriate. Look at the other ADRs for examples. However, the initial Table of metadata and header format is required to match.
Please see the [template](./adr-template.md). The template body is a guideline. Feel free to add sections as you feel appropriate. Look at the other ADRs for examples. However, the initial Table of metadata and header format is required to match.
Copy link
Contributor

@ripienaar ripienaar Jan 22, 2025

Choose a reason for hiding this comment

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

curious why specifically ./x, when relative worked fine before? Browsers will just remove it again when building the links

Copy link
Contributor Author

Choose a reason for hiding this comment

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

./ is that it is a tad more expressive, and GitHub will still link properly1.

./ is more easily searched for when relative links containing files in the same directory are ever moved into other locations. e.g. if a particular ADR, or all ADRs, are moved to their own sub-directory.

I am entirely happy with removing ./ from all links if this reasoning doesn't merit the change, but I think there are some upsides to using ./, from a maintainability perspective.

Footnotes

  1. GitHub even scrubs ./ from the URL...

Copy link
Contributor

Choose a reason for hiding this comment

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

These are fine examples of why, but no contributor will do this as its unusual :) I prefer to stick to normal paths that is likely to match what contributors would also do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is fair, I'll be glad to rewrite the commit to omit ./. I don't think any contributor even had a common method for linking other ADRs, as it was a bit all over the place - so people might end up linking to a GitHub page in the end after all 🤣

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed many ways, but none of them ever used ./ which suggests it's pretty certain they won't in future either :)

@bannatech bannatech force-pushed the banna/normalize-links branch from 6fa89ad to b93f987 Compare January 22, 2025 13:57
@ripienaar
Copy link
Contributor

ripienaar commented Jan 22, 2025

If you're feeling generous here are a few other broken things.

  • ADR-10.md links to 0007-error-codes.md which I suppose should be ADR-7.md
  • ADR-2.md links to 0001-jetstream-json-api-design.md which should be ADR-1.md

@bannatech bannatech force-pushed the banna/normalize-links branch from b93f987 to 451f76d Compare January 22, 2025 14:17
@bannatech
Copy link
Contributor Author

If you're feeling generous here are a few other broken things.

  • ADR-10.md links to 0007-error-codes.md which I suppose should be ADR-7.md
  • ADR-2.md links to 0001-jetstream-json-api-design.md which should be ADR-1.md

Ah, these flew right past me - I've fixed those links too. Thanks!

@ripienaar ripienaar merged commit bc6f698 into nats-io:main Jan 22, 2025
1 check passed
@ripienaar
Copy link
Contributor

Thank you @bannatech

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