Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Handle I18n case #4486

Merged
merged 5 commits into from
Mar 26, 2019
Merged

Handle I18n case #4486

merged 5 commits into from
Mar 26, 2019

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Jan 29, 2019

PR checklist

Overview of change:

Interface name (never-prefix) doesn't return error for I18nSomething

CHANGELOG.md entry:

[bugfix] interface-name now handles interface starting with "I18n" correctly

@reduckted
Copy link
Contributor

Instead of adding a special case just for "I18n", would a better solution be to check if the name without the "I" is a valid identifier? "18nFactory" isn't a valid identifier because it starts with a number, so the "I" prefix must be allowed.

This, of course, wouldn't work for "IDB", though I'm not sure why that was added as a hard-coded special case in the first place.

@VincentLanglet
Copy link
Contributor Author

@reduckted It seems to hard to check what is a valid identifier.

I cant say every number are not valid identifier, because 3rdParty, 1rstItem, 2ndElement can be.

@reduckted
Copy link
Contributor

I cant say every number are not valid identifier, because 3rdParty, 1rstItem, 2ndElement can be.

None of those are valid identifiers.

In JavaScript, identifiers can contain only alphanumeric characters (or "$" or "_"), and may not start with a digit.
- Identifier, MDN

So, what is allowed in an identifier name?

An identifier must start with $, _, or any character in the Unicode categories “Uppercase letter (Lu)”, “Lowercase letter (Ll)”, “Titlecase letter (Lt)”, “Modifier letter (Lm)”, “Other letter (Lo)”, or “Letter number (Nl)”.
- Valid JavaScript variable names in ES5

@VincentLanglet
Copy link
Contributor Author

@reduckted Ok, need to do a regex so.

BTW, IDB was added as a short cup for IndexedDataBase, i think.
It's debatable but I'll keep it to not create regression/breaking change.

@JoshuaKGoldberg
Copy link
Contributor

/cc @adidahiya, from the linked issue.

@adidahiya
Copy link
Contributor

Yeah, I agree the existing hard-coded "IDB" special case is weird, but we should keep it around to avoid regressions.

would a better solution be to check if the name without the "I" is a valid identifier?

sounds good to me

@VincentLanglet
Copy link
Contributor Author

After some reflexion, I'm not sure it's a good idea to check name.substr(1) is a valid identifier to know if name has a I prefix.

IDB case can be leggit. You don't want to forbid acronym. This way :

  • Two letters words may be allowed : ID can be acronym for IndexedData.
  • Three letters (+) words starting by two uppercases should be allowed if the third letters is also an uppercase. Like IDB or IDKWhatIsThis. We should also allow ID42, ID_42 or ID$42.

A good rule seems to be :
hasPrefixI: name[0] = 'I', name[1] Uppercase, name[2] Lowercase

And best way to check if name[1] is uppercase (and avoid special numeric or _, $ cases) is to check that name[1] is not lowercase. In the same way, I check name[2] is not uppercase.

@@ -74,6 +74,5 @@ function walk(ctx: Lint.WalkContext<{ never: boolean }>): void {
}

function hasPrefixI(name: string): boolean {
// Allow IndexedDB interfaces
return name.length >= 2 && name[0] === "I" && isUpperCase(name[1]) && !name.startsWith("IDB");
return name.length >= 3 && name[0] === "I" && !isLowerCase(name[1]) && !isUpperCase(name[2]);
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add some test cases in always-prefix/test.ts.lint as well? I feel like this will affect that option's behavior.

with always-prefix, I think the behavior should be:

  • failures: ID
  • valid: IDB (or, any all-caps interface name starting with I)

I know it feels a little silly since these are edge cases in naming, but we should try to be consistent in some way

Copy link
Contributor Author

@VincentLanglet VincentLanglet Feb 15, 2019

Choose a reason for hiding this comment

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

I can add test yes.
But I didn't change the behaviour for the always-prefix rule.

Actually the test check only if the first letter is I.
Now I'm thinking about it, an interface called Information should return an error but it doesn't.

With always-prefix, I think ID should return no error.
You can create an A interface, called IA with prefix. The same way a D interface would be named ID with prefix.

To be consistent the best way is to check
always-prefix && !hasPrefixI && !EdgeCase or never-prefix && hasPrefixI
Where EdgeCase are two letters upercase interface starting by I.

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Feb 23, 2019

@adidahiya @JoshuaKGoldberg I added a lot of test and then fix the edge cases.

@adidahiya adidahiya self-assigned this Feb 25, 2019
@styu
Copy link

styu commented Apr 10, 2019

I think this change caused a regression, I'm noticing now that correctly prefixed interfaces that have a number in it (e.g. IS3Foobar) is now returning errors

@VincentLanglet
Copy link
Contributor Author

@styu I already made a fix here #4626, I'll check it fix your case

@VincentLanglet VincentLanglet mentioned this pull request Apr 11, 2019
3 tasks
@styu
Copy link

styu commented Apr 11, 2019

thanks so much!

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

Successfully merging this pull request may close these issues.

5 participants