-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(eslint-plugin): add rule naming-conventions #1318
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
Conversation
Thanks for working on this! 🙂
|
Just pushed it. I didn't think anyone was looking just yet, so I had some local commits.
I got rid of some modifiers. I didn't hugely see the point of many of them. I figure it's better to implement a useful subset rather than go for 1:1 implementation, when it's already a really complex rule.
I removed a selectors:
Also I removed the deep nesting and extension based design of the selectors (i.e. the original had I still included some grouping selectors ( Finally, I have removed the complete power of defining a custom format regex (i.e. you must use a predefined format) for this first cut. I think this feature needs a lot of thought put into it because all of the predefined formats are actually really hard to express in regex. We can revisit this later.
I was thinking about this - I think the original implementation was missing a way to ban prefixes (right now it only lets you assert there is one of a set). Thinking about adding this in (so I can completely deprecate
The
Right now this rule won't actually check import statements at all. Looking at the original code, I don't think it did either.
Included this in the last push.
yes times 4
Yes and no... If the rule goes out with the current config for only checking for the existence of prefixes (not being able to ban them), then I would want to keep this rule, but cut it right back to only be an unconfigurable rule that does "ban I prefix" on interfaces. If I enhance it to allow banning, then yes, will be deprecated. |
Makes sense to me.
I liked the look of this feature, but perhaps in practice it's not all that important. I'll have to see what my configuration looks like when this rule is released. I understood it like this:
i.e. If you don't override settings for And But ya, I can imagine it being quite tricky to implement.
It's difficult to predict everyone's use cases. I know that camelCase checking is quite difficult with regular expressions, but there are other things that it could be useful for. At the moment I use a regex like
Regular expressions may not be able to solve all problems, but they are very flexible.
I guess it was possible with regular expressions ... something like
Ah right, got it. He's not using strict camelCase though, so this probably wasn't even necessary. I'll probably need it though.
Oh, okay. Cool.
Makes sense, I agree. |
Oh, I wanted to ask about #816 – would it be easy to add support for this? If |
Should also close:
Could also close (with some extra work): |
Unfortunately not, because JSON Schema doesn't allow for RegExp objects.
I think for the first cut, probably not.
Yeah I want to avoid regexes where possible. Usually it's better to be explicit.
I already documented the affix trimming, but I didn't explicitly state in the docs that an empty string matches all formats. You can get an empty string name if you trim all underscores/affixes. I.e.
I'm not sure how many people actually care to enforce that "you only prefix a parameter with an underscore if it's unused", vs just allowing it all the time.
Something that can easily be added in later. |
278a25a
to
8e0f14c
Compare
Oh, that's unfortunate. Anyway, you did end up closing it. 😄
Fair enough. Once this is merged, I'll update #816.
If you used If you're also requiring PascalCase, then I suppose it could "look ahead" and see that after stripping the
My regex actually shouldn't have included the 3rd a) The regex limits it to one word ( Not really a big deal for me, but as I said, I have no idea what strange naming conventions other developers will have. I guess we'll find out soon enough. 🙂
It would bug me if developers used underscore-prefixed parameters, when all/most other identifiers were strictCamelCase. It also wouldn't be obvious which parameters were unused, if you couldn't trust that underscores were only allowed on unused parameters. It's the same reason for having the |
Hmmm, good point. I'll have to think on this more, damn.
That's my plan. I'm hoping that releasing this will bring all of the weird people out of the woodwork to give concrete examples of wanting very specific naming conventions.
Yeah I get you. I.e. using inbuilt TS logic, I can enforce that unused vars must have an underscore, but I can't enforce that used vars do not have an underscore. The only option for doing it is the manual analysis 😢 |
Is it worth submitting an issue to the TS project about this? |
8e0f14c
to
22d6773
Compare
note: added option This was the last blocker I had for this rule, so if everyone is happy, I'm good to merge this. |
Codecov Report
@@ Coverage Diff @@
## master #1318 +/- ##
==========================================
- Coverage 94.45% 94.09% -0.36%
==========================================
Files 142 143 +1
Lines 6100 6457 +357
Branches 1736 1823 +87
==========================================
+ Hits 5762 6076 +314
- Misses 183 204 +21
- Partials 155 177 +22
|
I'll merge this in before the release tomorrow morning (in like 13 hours). It's a rather large rule with a lot of deprecations, so I just want to make sure everyone agrees its in the right spot before it goes in. |
... after updating to 2.16 I get:
.. and I also cannot disable it in my project via |
Got the same error when using
|
Same here:
|
Also here:
|
Hey guys, thanks for the reports. Just a heads up for how we like to do things around here:
|
Fixes #515
Fixes #590
Fixes #671
Fixes #722
Fixes #871
Fixes #1145
This is a simplified implementation of the
naming-conventions
rule fromtslint-consistent-codestyle
.Unifying all of the naming rules into once place, and adding a lot of options.
Configuration will look something like: