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

[no-unlocalized-strings] Tracking issue for the rule improvements. #50

Closed
5 of 6 tasks
timofei-iatsenko opened this issue Oct 10, 2024 · 10 comments · Fixed by #78
Closed
5 of 6 tasks

[no-unlocalized-strings] Tracking issue for the rule improvements. #50

timofei-iatsenko opened this issue Oct 10, 2024 · 10 comments · Fixed by #78
Assignees
Labels

Comments

@timofei-iatsenko
Copy link
Collaborator

timofei-iatsenko commented Oct 10, 2024

I created this issue to collect all problems / improvements for no-unlocalized-strings rule.

@jgarplind
Copy link

Not sure if this is a good place to share input, but we noticed that const CAPITAL_LETTERS is always ignored(?).

Might be an anti-pattern on our end, but we have occasionally used such syntax for copy text, which the linter did not help us catch.

@timofei-iatsenko
Copy link
Collaborator Author

Yes, this excluded by default. The capital letters in the variable names mostly used to define constants, so they are excluded.

I think it could be a good idea to revisit the defaults, and make them configurable or overridable.

@nyadav810
Copy link

Any chance that regex patterns will be supported in ignoreFunction in the future? I want to be able to ignore Events.*, logger.* and console.* with all functions exposed on a certain object ignored by this rule. Thanks!

@timofei-iatsenko
Copy link
Collaborator Author

@nyadav810 ignoreFunction is a little bit different from other ignore* options, because ignoreFunction supports complex selectors matching object.method, which in AST is presented as different nodes, so the signature for this option is not trivial.

@timofei-iatsenko
Copy link
Collaborator Author

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.

Let me know what you think, and feel free to share any other feedback you have on this rule.

@jgarplind
Copy link

I think the defaults so far have been very sane, and only small caveats such as the above have been slight nuissences.

As long as it is easy to reproduce the existing default experience, I think that sounds good!

@timofei-iatsenko
Copy link
Collaborator Author

@nyadav810 FYI #77

@timofei-iatsenko
Copy link
Collaborator Author

Guys, I would appreciate if you review and left your feedback on this PR #78

Also while working on it, i've noticed that in a real life sceanario we probably don't need separate settings for ignoreAttribute, ignoreProperties, ignoreVariable because in most of the cases we will want to have the same values in all of them. And now i'm thinking of replacing all these 3 options into one.

/* eslint lingui/no-unlocalized-strings: ["error", {"ignoreAttribute": ["className"]}] */
export const MyComponent() {
 return <div className="classes string">Ola!</>
}
/* eslint lingui/no-unlocalized-strings: ["error", {"ignoreVariable": ["className"]}] */
export const MyComponent() {
  const className = "doing some magic";

 return <div className={className}>Ola!</>
}
/* eslint lingui/no-unlocalized-strings: ["error", {"ignoreProperty": ["className"]}] */
const variants = {
  variantA: {
    className: "classes string"
  },
  variantB: {
    className: "classes string"
  }
]
export const MyComponent({variant}) {
 return <div className={variants[variant].className}>Ola!</>
}

@nyadav810
Copy link

nyadav810 commented Nov 12, 2024

Thanks for the improvements @timofei-iatsenko !!!

Another quirk I found - when called as a function, msg triggers this warning as well!

Example:

msg({ message: "hello", comment: "a common greeting" })

...
warning  String not marked for translation. Wrap it with t``, <Trans>, or msg``  lingui/no-unlocalized-strings

(I'm having to add msg to ignoreFunction in the ESLint config to ignore those)

@timofei-iatsenko
Copy link
Collaborator Author

@nyadav810 thanks for the report. Also noticed it when worked on the #78. It's fixed there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants