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

feat(no-unlocalized-strings): remove default configuration #78

Merged
merged 5 commits into from
Nov 14, 2024

Conversation

timofei-iatsenko
Copy link
Collaborator

@timofei-iatsenko timofei-iatsenko commented Nov 4, 2024

Quote myself:

I went through the issues with this rule and tried to address them all. It seems like we need a big change in how the rule is set up. There’s no way to make it work perfectly for everyone and every project. So, I’ve decided to remove all the built-in settings and made them fully customizable.

For example, if a user doesn’t want:

but we noticed that const CAPITAL_LETTERS is always ignored(?).

they can simply remove the related regex from their options.

This will fix

And possible many more issues in the future regarding the defaults.

What was done:

  1. All hard-coded default are removed in favor of user configuration
  2. Main words regex was extended to support non-Latin letters, such as Chinese or Cyrillic.
  3. ignoreAttribute, ignoreProperty and ignoreVariable options were merged into single ignoreNames
  4. strictAttribute option removed. It didn't work properly, and now it's not needed because the user can override defaults more naturally (that was an original request for this option).
  5. rule is not triggered for lingui's msg and selectOrdinal functions

Breaking Changes

  1. The rule will not have any defaults. You need to set defaults in eslint config for this rule. Refer to rule documentation for the basic configuration.
  2. The ignoreFunction option was renamed to ignoreFunctions
  3. ignoreAttribute, ignoreProperty and ignoreVariable options were merged into single ignoreNames
  4. strictAttribute option removed.

Copy link

codecov bot commented Nov 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.49%. Comparing base (e249254) to head (0bfae9b).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #78      +/-   ##
==========================================
- Coverage   96.61%   96.49%   -0.12%     
==========================================
  Files          10       10              
  Lines         413      371      -42     
  Branches      118      101      -17     
==========================================
- Hits          399      358      -41     
+ Misses         14       13       -1     

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

@timofei-iatsenko timofei-iatsenko force-pushed the feature/remove-default-configuration branch from 8b694e5 to f70111e Compare November 4, 2024 12:15
@timofei-iatsenko timofei-iatsenko marked this pull request as ready for review November 4, 2024 12:16
@@ -47,6 +47,8 @@ export default [
]
```

We also recommend enabling the [no-unlocalized-strings](docs/rules/no-unlocalized-strings.md) rule. It’s not enabled by default because it needs to be set up specifically for your project. Please check the rule's documentation for example configurations.

Choose a reason for hiding this comment

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

I can see why you want to go in this direction, but personally I had a lot of help from the defaults, and might have been hesitant to even try to use the rule if faced with a custom setup.

UPPERCASE_LETTERS being ignored was fine imo. My 10/10 experience would be what we had, including the ability to override such specifics if desired (unsure how feasible that is).

Either way, I'm a bit wary here, thinking that this PR may be a move away from a 9/10 out of the box experience which frankly is pretty good.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem is a could not come up with a good way to override defaults. So we have two situations:

  1. When user ok with provided defaults and want to add more specificity for the project
  2. User is not ok with defaults and want to revert some of the buil-in rules (that already happened few times, saying from issues)

First case is already supported, we can extend the defaults.

The second case is a bit trickier, you need to give a way for user to choose what is going to happen, override or extend? This either brings more complicated structure of every option or double of options (ignorePoperty, ignorePropertyOverride). I don't like both of variants, actually.

That's why I decided to give a blank list, so there is nothing to override and it's pretty configurable. Also, it gives much more clarity about what exactly is ignored

Choose a reason for hiding this comment

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

Solid reasoning.

Your example is thorough and should be easy enough to copy paste and run with. I might have been further encouraged if this language

You can use the following config as a starting point and then adjust it for your project

was slightly more assertive, i.e. officially recommending it, or mentioning it's history as the de facto-rule set before opening up for configuration.

@timofei-iatsenko
Copy link
Collaborator Author

@andrii-bodnar this one is ready to be merged.

Copy link
Contributor

@andrii-bodnar andrii-bodnar left a comment

Choose a reason for hiding this comment

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

@timofei-iatsenko thank you!

@andrii-bodnar andrii-bodnar merged commit 32c823c into main Nov 14, 2024
6 checks passed
@andrii-bodnar andrii-bodnar deleted the feature/remove-default-configuration branch November 14, 2024 09:15
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.

[no-unlocalized-strings] Tracking issue for the rule improvements. Support Chinese Please
4 participants