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

openapi: sort tags file #4595

Merged
merged 5 commits into from
Sep 1, 2023
Merged

openapi: sort tags file #4595

merged 5 commits into from
Sep 1, 2023

Conversation

thomasheartman
Copy link
Contributor

@thomasheartman thomasheartman commented Sep 1, 2023

This change sorts the tags in the tags file and tests that the list is sorted alphabetically. This makes it easier to
find tags in the file.

#4580 already introduced a test to check that we have no duplicate
tags, so this isn't as necessary anymore, but it's still nice to have.

It also removes the previous auto-sorting before exporting. This is to ensure that entries are sorted in the source list. This might seem like a regression, but it makes it easier to spot near-duplicate tags:

Despite having the test that validates there are no duplicates, you can always have Notifications and Notification API by mistake (tags that mean the same but are different). Keeping the list alphabetically sorted might help to prevent this before pushing the change to prod. In this case, we will eventually find out and fix it, so this could be a good reason to have the list sorted.

This change sorts the tags in the tags file. This makes it easier to
find tags in the file.

#4580 already introduced a test to check that we have no duplicate
tags, so this isn't as necessary anymore, but it's still nice to have.
@vercel
Copy link

vercel bot commented Sep 1, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
unleash-docs ⬜️ Ignored (Inspect) Visit Preview Sep 1, 2023 10:13am
unleash-monorepo-frontend ⬜️ Ignored (Inspect) Visit Preview Sep 1, 2023 10:13am

Copy link
Contributor

@gastonfournier gastonfournier left a comment

Choose a reason for hiding this comment

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

Looks great! We could also add a test to validate this is sorted, so we don't break it in the future

@thomasheartman
Copy link
Contributor Author

Ooh, I like that. Give me a bit to whip it up 😄

@thomasheartman
Copy link
Contributor Author

On the other hand, we already sort it before exporting it, so it's never going to be an issue in the output. I'm not sure how I feel about exporting the raw constant and checking that. It seems like an arbitrary thing to cause test failure over. What do you think?

@gastonfournier
Copy link
Contributor

On the other hand, we already sort it before exporting it, so it's never going to be an issue in the output. I'm not sure how I feel about exporting the raw constant and checking that. It seems like an arbitrary thing to cause test failure over. What do you think?

🤔 The question I'd ask myself is What's the value of having it sorted in this file if we're already sorting it later?, and 2 things come to my mind:

  1. Performance: We could remove the sort afterward to improve performance (provided the test guarantees the list is already sorted). But this feels like a very soft reason.
  2. Correction: Despite having the test that validates there are no duplicates, you can always have Notifications and Notification API by mistake (tags that mean the same but are different). Keeping the list alphabetically sorted might help to prevent this before pushing the change to prod. In this case, we will eventually find out and fix it, so this could be a good reason to have the list sorted.

@thomasheartman
Copy link
Contributor Author

Yeah, okay, I like your reasoning here. You've convinced me. I'll fix.

@thomasheartman thomasheartman enabled auto-merge (squash) September 1, 2023 10:15
@thomasheartman thomasheartman merged commit 1799086 into main Sep 1, 2023
6 checks passed
@thomasheartman thomasheartman deleted the openapi/sort-tags-file branch September 1, 2023 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants