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

chore(types): narrow string to Locale where possible #11323

Merged
merged 10 commits into from
Jun 24, 2024

Conversation

C17AN
Copy link
Contributor

@C17AN C17AN commented Jun 17, 2024

Summary

Problem

As the locale key value is widely used in the project, I thought it would be better to manage it more thoroughly.

Solution

Narrow locale property's type from string to Locale.

Before

image

After

image

@C17AN C17AN requested a review from a team as a code owner June 17, 2024 12:15
@C17AN C17AN changed the title chore: Narrow 'string' type assigned to the 'locale' variable to 'Locale' type fix: Narrow 'string' type assigned to the 'locale' variable to 'Locale' type Jun 17, 2024
@C17AN C17AN changed the title fix: Narrow 'string' type assigned to the 'locale' variable to 'Locale' type fix: Narrow 'string' type of 'locale' property to 'Locale' type Jun 17, 2024
@C17AN
Copy link
Contributor Author

C17AN commented Jun 17, 2024

While correcting the Lint error, I realized that more changes were needed than I thought.

I think it would be meaningful to change the string type to the Locale type, but there is a concern that this change could lead to a breaking change.

I would appreciate it if you could feel free to leave your comments on what you think.

@github-actions github-actions bot added the markdown markdown related issues and pull requests label Jun 17, 2024
@C17AN C17AN force-pushed the chore/Locale-typing branch 2 times, most recently from 176a036 to 72829bb Compare June 17, 2024 13:18
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions bot added the merge conflicts 🚧 Please rebase onto or merge the latest main. label Jun 19, 2024
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

Overall LGTM, but two nits.

content/redirect.ts Show resolved Hide resolved
markdown/m2h/cli.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the merge conflicts 🚧 Please rebase onto or merge the latest main. label Jun 24, 2024
@caugner caugner changed the title fix: Narrow 'string' type of 'locale' property to 'Locale' type chore(types): narrow string to Locale where possible Jun 24, 2024
@C17AN
Copy link
Contributor Author

C17AN commented Jun 24, 2024

Thank you for your review, @caugner!
If there is anything else I need to do, please feel free to let me know, and have a great day!

@C17AN C17AN requested a review from caugner June 24, 2024 12:33
@caugner caugner merged commit 3a20751 into mdn:main Jun 24, 2024
9 checks passed
@caugner
Copy link
Contributor

caugner commented Jun 24, 2024

If there is anything else I need to do, please feel free to let me know, and have a great day!

Thank you, we're all good, and I just merged! 🎉

@C17AN C17AN deleted the chore/Locale-typing branch July 3, 2024 07:27
ferdnyc pushed a commit to ferdnyc/yari that referenced this pull request Jul 13, 2024
Co-authored-by: Claas Augner <caugner@mozilla.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
markdown markdown related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants