feat(rules): prefer-valid-title (#273) #433
Conversation
suganya-sangith
changed the title
suganya-sangith
force-pushed
the
valid-title-rule-273
branch
from
be1ba8b
to
e72ba01
Compare
There was a problem hiding this comment.
Looking good! Thanks a ton for this - I've been after this for a while, but not gotten the time to tackle it myself 😄
I've done an initial review, and so far it's all looking pretty good - I've left a few changes, but will do a second review once I'm home from work.
|
@G-Rath Thanks for taking time to review it, will work on the suggested changes 👍 |
suganya-sangith
force-pushed
the
valid-title-rule-273
branch
2 times, most recently
from
83e4754
to
83fee7d
Compare
|
@G-Rath Made all the changes, expect getAccessorValue for which I could not find an alternate function to get the title. |
src/rules/valid-title.ts
Outdated
| return null; | ||
| } | ||
|
|
||
| return getAccessorValue(argument); |
There was a problem hiding this comment.
This should be getStringValue rather than getAccessorValue.
A StringNode is a union type of either StringLiteral or TemplateLiteral, where as an AccessorNode is either a StringNode OR a KnownIdentifier.
AccessorNodes represent nodes that are accessing a property of an object, which isn't what you want in this case :)
This is totally on me btw: I checked the code base, and I've used getAccessorValue in a few places where I should have used getStringValue 😬
I'm happy if you want to leave this one, and I can look to clean it up when I do my refactor later this weekend :)
There was a problem hiding this comment.
Thanks for the explanation 👍 Will spend time to understand the types available 😁
There was a problem hiding this comment.
No problem - I wrote all of the types and guards, and still get confused.
It's actually why I wrote them so grainaulay: The nature of AST parsing means it's best to not try and do fancy things, and instead layer everything to build on the previous type.
As such, the types are something like:
A StringLiteral is a Literal whose value is of type String (i.e 'hello world').
A TemplateLiteral is a Template Literal, that has only one quasis (i.e `hello world`)
quasisis a property onTemplateLiterals, that is an array ofTemplateElements.- No idea why it's
quasis- it's called that in the spec, but I can't find a definition of the word.
- No idea why it's
- In theory this should mean only "simple" template literals that don't have any expressions, as a
TemplateElementsort of represents the "stringy" bits of a template literal i.e`hello ${world}`has 2TemplateElements, the first beinghello ${(w/ a value ofhello) and the second being}(w/ a value of''). - In practice, this does work, but I think it might be better to check the length of
expression, to see if it's 0- I know
no-identical-titlechecks bothisStringNode& the expression length if it's a template, and fails if I remove the expression length check, which iirc meansisStringNodeisn't doing its job right.
- I know
A StringNode is a node that's either a StringLiteral OR a TemplateLiteral.
That ends the "string" side of nodes. You then have Identifiers, which are pretty much what they say on the tin: they're something that "identifies" as something - best way to think of them is as plaintext "names" or "words" - anything else is another expression, i.e [] is an ArrayExpression.
Identifiers are what you use to access properties, methods, fields, etc via dot notation. They're also how variables are used.
A KnownIdentifier is an Identifier whose name we "known"
- This is actually a bit of a cheeky one - when we say "known", the "least known"
Identifierhas anameof typestring; that's it. Which the case for allIdentifiers anyway, and is the default. The name was more about reducing confusion between our generically typedIdentifierandTSESTree.Identifier, of which ours extends.
An AccessorNode is then either a StringNode OR a KnownIdentifier.
The whole backstory behind AccessorNode is that you can access things in two ways in JS: Dot notation, and Bracket notation.
Dot accessors are Identifiers - nice and easy. Bracket accessors however are done using "a string" in square brackets; so they're StringNodes (b/c they could be string literals or template expressions - they can also be Identifiers themselves, b/c variables but we don't support that b/c we're not trying to be TS 😉).
As such, we have isSupportedAccessor, which guards on if a node is an AccessorNode that we "support" - which simply means we have an implementation for resolving the value of the node, which can be done via getAccessorValue, which acts like a normaliser for getting of the node you're working w/.
And there you go, that's pretty much it in a nutshell 😄
While on the surface it can look very complex, it's actually pretty simple to the point of being elegant (IMO at least 😉) - it's done as just a composition chain, which means you just have to follow the type chain down each step to get an idea of what the previous step is made up of.
In saying that, once you throw the actual AST tree into the mix, it's very easy to get confused & mentally tongue tied. The above is covering only the property accessor side; theres also the expect/modifier/matcher utils, which are also hopefully as elegant, but still their own kettle of fish.
Finally, I'm meant to be writing a whole new set of parsers/guards for the jest globals (describe, test, it, etc), but that's slow going for now (plus what we have is working, so it's less needed).
This is pretty much the best write up I've done on this so far, and something I've been meaning to do for a while. Cheers!
There was a problem hiding this comment.
Wow, that's so much of information, Thank you very much for taking time to write this up 😊
There was a problem hiding this comment.
No problem! I've tried to write this up a few times before, but didn't get very far, so when I started writing this comment I just let it snowball so that as much info as possible is on paper 😂
suganya-sangith
force-pushed
the
valid-title-rule-273
branch
from
83fee7d
to
3a60287
Compare
There was a problem hiding this comment.
LGTM 👍
So glad we're going to have this!
I'll probably chuck it into the recommended rules at some point, unless @SimenB or @jeysal have any objections?
I can't think why you'd recommend having tests that are test('test something', ... or it(' should not throw', ....
@suganya-sangith Thanks again! If you don't mind me meddling so soon after merging, I'll probably look to expand this rule over this weekend, as it's opened the way to resolve some issues that've been open for a while, as well as give the code base a good clean :D
|
Yeah seems like a recommended candidate for next major version to me as well |
|
🎉 This PR is included in version 22.20.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
| @@ -0,0 +1,65 @@ | |||
| # Disallow duplicate setup and teardown hooks (no-duplicate-hooks) | |||
There was a problem hiding this comment.
This title seems to be invalid. 😊 (probably copy-paste mistake)
There was a problem hiding this comment.
@mrtnzlml Thanks for catching it. I have used the template from other read me to maintain the consistency, but missed to change the title 😬
As part of this, added a valid-title rule which checks for accidental space and duplicate prefix
@G-Rath Kindly review the PR and let me know if there are any changes