-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
remove @typescript-eslint/interface-name-prefix from recommended #374
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
Comments
The recommended that has been setup is based off a number of things, namely (in this case) what is done in the official typescript documentation/codebase. The reason this is recommended as never adding a prefix is because prefixing your interface names (esp with People generally fall into this pattern because they have come from languages like C#/Java where the convention is to prefix interface names with In TS there's no technical reason you need to differentiate the naming of That being said, if there is drive from the community, we can remove it. |
agreed to keep it! just bring it up since I was start using it in an existing repo. maybe the docs can be improved(as you described).
|
We can definitely add something to the docs! For those that are giving my comment a 👎 (and the OP a 👍) - please let me know your use case for specifically giving I don't see any reason to specifically prefix interface Foo {
prop: string
}
// the same
type TypeBar = Foo & {
prop2: number
};
interface InterfaceBar extends Foo {
prop2: number
} type Foo = {
prop: string
}
// the same
type TypeBar = Foo & {
prop2: number
};
interface InterfaceBar extends Foo {
prop2: number
} Considering more or less |
We prefix with It would be great to have a better solution at the language / packaging level since it is always ambiguous when you import |
My main issue with the rule is the inherent problem of false positives. An otherwise perfect codebase will still be linted if it contains types like |
@ThomasdenH False positives like that are a bug, and are easily fixed in the rule's regex match. @rstrom The general (and much clearer solution) I've seen for that problem is not to use named imports, instead doing the following: This leads to a better convention than just import * as GQL from 'my/generated/types';
import { Entity, IEntity } from './component';
// GQL.Entity can never collide with Entity
// GQL.Entity can never collide with IEntity
// always clear which entity I want as I establish the convention that I've "namespaced" the graphql types
// versus
import { IEntity } from 'my/generated/types/I';
import { Entity, IEntity } from './component';
// because you've only prefixed it with an I, you don't know which IEntity to use and when.
// no namespacing so hard to tell which is which and in what case to use which one.. It's pretty trivial to write an internal eslint rule to enforce always using
There is no way to differentiate between them at code-time, no. Flow uses the
|
Thanks @bradzacher that's a good idea! One drawback with that approach is that the existing integration with an IDE like VS Code... If you start typing |
@bradzacher What I meant by my comment is that it's inherently impossible to know whether the starting 'I' is part of the name or not. The name You might argue that starting with a single capital is not idiomatic anyway, but then the scope of the rule changes a lot. To be consistent other capitals would need to be caught as well. So if a perfect codebase might require error surpressions for the rule, I'd argue against having it in the default rule set. |
I see your point, though I would argue that is quite an edge case that can be fixed by using a better, clearer naming convention. The general use case is this: if you're writing interfaces that start with a capital I followed by a second capital letter, then you're following the anti-pattern of using I-prefixed interface names. If you, as a user, know that your project uses shorthands that mean it's legitimate for your interface names to start with |
However this discussion will be resolved, I would still recommend to remove this line from the README:
Simply because on the very page you link to, they write twice that:
Speaking about which - @bradzacher would you mind linking to some sources where it is said that this is anti-pattern and why? (I respect your arguments but you mentioned this right in the beginning already, so I assume it's not just you who makes them) |
I'd suggest to make it a warning instead an error. Or even creating another plugin with only stricter rules (I based off |
IMO, the
Granted that style guide is intended for internal use within the Microsoft/TypeScript repo, but given the authors of TypeScript take the opposite stance, it probably shouldn't be default to require I also very rarely see interfaces prefixed with I'm not saying the default should take a stance either way - "always |
This is not an argument for removal from the recommended set, as the entire point of the recommended set is to provide an opinionated default set of rules so that the user doesn't have to think about it.
Our recommended set is based off of typescript best practices, and the best practice (as you and others have stated) is to not prefix with |
If people can provide a concrete argument as to why it should not be recommended, or if there is significant drive from the community, we're happy to remove it. Right now we have the following reasons:
|
I would go by what Microsoft themselves do and what is mostly seen in the wild. I've seen perhaps one package out the many I've used, zero with what our company currently uses. With that said, it might be easy to say "well then the recommendation should be never I see recommended more as "sane defaults" that you can build on. I shouldn't need to override a bunch of rules. Instead, I should be adding to the rules that are not currently in recommended. |
I'd love to know if people do, as that would be a good case to remove it from the recommended. Based on those numbers, we're a long way from concrete evidence that I'm looking for to justify it's removal (which involves a major version bump as it is a breaking change).
As mentioned - our recommended is not just a minimal set, but is instead a set designed instead to enforce best practices. Users reach straight for the recommended set by default when installing a new plugin, and we want the "default" experience to be that set of best practices.
The way I see it; if a project overrides a bunch of rules from our recommended set then either:
|
14k results where 10k results where 25k results with recommended with no mention of 1k results that set From the above, 48% of don't consider this a best practice enough to change it on their own. 28% went as far as choosing The search is not perfect. It also doesn't count Bitbucket, GitLab, etc. Or private projects. Significantly more than 20 though. |
My point with saying that is that people are arguing for its removal because "people don't agree with it" without any stats behind it. So I was pointing out that opinions are cool and great to have, but how are people actually using it - what are the data points behind it? The requestors are trying to convince us to make/accept the change. But we're not going to be convinced without good arguments or data to back up the request, and we're not going to do the research on their behalf... Thanks heaps for going and getting some data! Though the searches are tslint instead of eslint, it's a good starting point (as the two rules do the same thing). Small correction to your "recommended with no interface-name" query: specifically searching for "tslint:recommended" dropped that number by half (I think due to the copy pasted comments I mentioned). So looking at it (note the numbers given are approximate because every refresh changes the total results by as much as 4k in either direction...). Total results 40.9k.
Another thing to note is that these searches reveal around 41k results, but also according to github search, there are over 840k tslint.json files. So the searches are somehow revealing only 4% of the dataset..... Ultimately I'm not sure how much stock to put into these results. Looking at some of the results themselves, they're a bit dubious in terms of quality:
It's too hard to tell if the above skews the results in one way or another, regardless I don't trust the numbers are at all accurate.
Could try and do an analysis of eslint users, but idk if it's worth the effort with github search being the way that it is.. |
Even with a 41k dataset, 60% is rather significant, is it not? Even if it were 40%, I would consider that high enough to take a neutral stance. This is "tab vs space, semi vs no-semi, double quote vs single" territory. |
The problem is that I have very low confidence in the quality of the data for the reasons I outlined above. A big problem which destroys my confidence in the results is the number of "demo" / "example" repos I saw in the results, which I believe heavily skews the number of "recommended without changing the setting" results. It's invalid data because it's not actual use of tslint. Those repos are something like: find webpage with some typescript tutorial, download the tutorial's code zip (which includes a tslint.json), follow along and make <10 commits to the repo, and then abandon the repo. The data paints a picture, sure, but it's a low confidence one which I don't want to base any decisions off of in of itself. Considering how low the volume of feedback is on github (12 👍's), my current stance is that it's not worth bumping for a breaking change. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@bradzacher I think the problem with this rule is that it was clearly created to break the habit, ingrained into C# and Java developers, of always prefixing interfaces. Specifically, it was targeted to Microsoft employees with a C# background, since this rue originates from the internal guidelines for Typescript. Now that Typescript is being massively adopted by web developers, who for the most part don't have such backgrounds, to forbid it feels like an arbitrary artefact inherited from another ecosystem. As you mentioned, it's true that it's not necessary to systematically prefix interfaces with Of course one could come up with another naming scheme, but again this |
Why would you disable an entire rule across your codebase just because there are some edge cases where you might want it off? Handling certain edge cases is exactly the reason that eslint has disable comments: // more ergonomic naming of the reusable interface
// eslint-disable-next-line @typescript-eslint/interface-name-prefix
interface IMyFoo {}
class MyFoo implements IMyFoo {}
export { IMyFoo, MyFoo } As a support example for this - you could make the same argument for the OR I can leave the lint rule on to enforce the style across my entire codebase, and add a disable comment for the one edge case: // the API provides snake_case properties, which we can't change
/* eslint-disable @typescript-eslint/camelcase */
interface MyApiType {
some_property: string;
}
/* eslint-enable @typescript-eslint/camelcase */ Another solution is to use a different, more explicit naming convention. As you said - the So you could just as easily use an explicit convention like: interface MyFooInterface {}
class MyFoo implements MyFooInterface {}
export { MyFooInterface, MyFoo } |
You can't enforce |
The authors of TypeScript put in their own style guide the opposite - Never prefix with |
So suffix with Interface is? You can't see the difference if something is an interface or a class. It sucks in code reviews if you can't see if someone use classes as dependency instead of the interface. class Example {
@inject(TYPE.MyService)
readonly myService!: IMyService;
} vs class Example {
@inject(TYPE.MyService)
readonly myService!: MyService;
} The most developers are using classes as hard coded dependency and use interfaces just for data objects. But that is not a good practice even if it is common. |
@hbroer - I've established this in other threads (such as #433 (comment)), but there's little to no difference between Do you prefix your To me your example makes no sense - one character difference is not easy to see and I know that most reviewers would miss that in a review. But you said that it sucks to write - well aren't you using an IDE with autocomplete, like vscode? Keystrokes is such a false argument now a days that developer tooling is getting so good with things like intellisense. |
i don't use I have autocomplete. It is faster if you just need to type |
This is where intellisense shines - typing So ultimately it's no faster to type when using developer tooling, and the name
One thing people don't realise is that It's actually trivial to write your own internal eslint rules to enforce whatever conventions you would like. For example, (I know it's going to be significantly faster for me to do because I have in-depth knowledge of eslint), but it took me about 5 minutes to write a rule with a fixer to enforce the suffix convention: |
I use intellisense with code reviews as well. We use Bitbucket, so we use the Atlassian plugin to do code-reviews inside of vscode. If I cared whether a type was an interface or a class I would need to follow the definition anyway. In general, for our team, an |
@bradzacher Obviously, disabling or working around the rule is always possible; the discussion should be whether it deserves to be a default. I still haven't heard a positive reason why this rule should be enabled, except to help C#/Java devs ease themselves into Typescript/Javascript. Outside of this historical context, the rule makes little sense. |
This rule exists to help prevent people from introducing a typescript anti-pattern into their codebase.
In this thread and others linked, I have put forward the following reasons that you should not use this naming convention:
The question I've been looking for an answer to is this: Why do you want to treat
Sure, I got a little silly toward the end, more or less leading into a slippery slope argument, but the questions are still valid. If you boil it down, the response I keep getting is "I want to use Your IDE will provide you ample tooling to help you differentiate everything above. So again - why provide Sure, there are a few edge cases (such as falsely reporting Similar to my example with the Ultimately, we are discussing the recommended set. Discussing what the recommended way to program TypeScript is. We are recommending that you don't follow this naming convention for the reasons I laid out above. The reason that I've left this issue open is that I'm hoping someone will bring an argument that isn't "because I want to". |
@bradzacher I agree with you that an interface should not be prefixed with The current default Sorry for the confusion on my end as I did not realize this until now. We are still in a transition period, and I was simply continuing the conversation from tslint. |
So, is there any way to have the rule that an interface MUST start with |
Please have a read of the rule docs for I'm going to close this. #651 outlines the recommeneded changes we're making for 2.0 release. We will not be removing |
consider removing
@typescript-eslint/interface-name-prefix
fromrecommended
, since it seems just a stylistic issue.Versions
@typescript-eslint/eslint-plugin
1.5.0
The text was updated successfully, but these errors were encountered: