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

i18n ESlint rules do not correctly default text-domain #21920

Closed
JimSchofield opened this issue Apr 27, 2020 · 6 comments · Fixed by #21928
Closed

i18n ESlint rules do not correctly default text-domain #21920

JimSchofield opened this issue Apr 27, 2020 · 6 comments · Fixed by #21928
Assignees
Labels
[Package] ESLint plugin /packages/eslint-plugin [Package] Scripts /packages/scripts [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@JimSchofield
Copy link

Describe the bug
When using @wordpress/eslint-plugin if no text domain is configured in eslintrc, 'default' should be the chosen text domain and there should be no text-domain linting errors with a supplied domain string or empty param.

To reproduce
Steps to reproduce the behavior:

  1. Install @wordpress/eslint-plugin
  2. Do not configure the @wordpress/i18n-text-domain" in eslintrc
  3. Try including a __( ) function in a javascript file with a text domain (e.f. __( 'some string', 'some domain' )
  4. Notice linting error @wordpress/i18n-text-domain: Invalid text domain 'some domain'

Expected behavior
Giving no preferred text domain in the eslintrc should allow any string text domain. OR Text domain rule should be outlined as required in the eslint plugin documentation.

Additional context

  • This is seen in a custom plugin using eslint-plugin
@gziolo
Copy link
Member

gziolo commented Apr 27, 2020

Good timing, I was thinking about a fix for it. I had to workaround it in WordPress/gutenberg-examples this way:
https://github.com/WordPress/gutenberg-examples/blob/c4d83b1dbd9c46de9028db10c3206446ed084b18/01-basic-esnext/package.json#L24

However, we need to find a more robust solution that works for all projects out of the box.

@gziolo gziolo added [Package] ESLint plugin /packages/eslint-plugin [Package] Scripts /packages/scripts [Type] Bug An existing feature does not function as intended labels Apr 27, 2020
@swissspidy
Copy link
Member

I think the easiest solution would be to remove the default text domain:

const { allowedTextDomain = 'default' } = options;

So when the text domain is not explicitly specified (allowedTextDomain === undefined), the rule would do nothing

Thoughts?

cc @aduth

@gziolo
Copy link
Member

gziolo commented Apr 27, 2020

@swissspidy, I tried what you shared in #21928. It works but I guess it can be simplified :)

@aduth
Copy link
Member

aduth commented Apr 27, 2020

I'd be motivated toward anything which ensures the default configuration can be used out-of-the-box without additional action required.

I guess #21920 (comment) can be a solution toward that, but it's unclear to me: Is there any difference between removing this default value, and simply removing the rule from the configuration altogether? If the rule isn't doing anything in its default state, then I don't think it should run at all.

@swissspidy
Copy link
Member

@aduth The rule also checks whether the text domain is a valid string literal and not a variable, for example. That's still valuable.

@aduth
Copy link
Member

aduth commented Apr 27, 2020

@aduth The rule also checks whether the text domain is a valid string literal and not a variable, for example. That's still valuable.

Okay, in that case, it seems sensible then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] ESLint plugin /packages/eslint-plugin [Package] Scripts /packages/scripts [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants