-
-
Notifications
You must be signed in to change notification settings - Fork 159
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
feat: added granular flat TypeScript configs #1298
feat: added granular flat TypeScript configs #1298
Conversation
290ab93
to
f2b1c1d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, looking very good so far.
I think we should also add these to requirements-typescript-flavor configs (which otherwise are the same as the requirements-typescript configs)
- 'jsdoc/require-param-type'
- 'jsdoc/require-property-type'
- 'jsdoc/require-returns-type'
As far as other rules, I don't want to bikeshed, but I think this is a good opportunity to look at the whole picture if you don't mind bearing with me here.
I think some may come to the configs to get a good overview of what the plugin does, so if they can get a comprehensive overview by seeing configs which encompass all the rules (if not the opportunity to include all, as requested by #701), I think this will be a good deal for them, so in that vein (and for its own sake), I might suggest we consider the following in addition.
(I know the following rules are disabled by default, but still worth considering too, imo, especially since some should probably be added to recommended but were awaiting a breaking change. Note that some have settings which may be more widely appealing than the default, e.g., requiring hyphens for descriptions has an option to disallow them. If we don't add the following, then I think it makes sense to at least mention them in a rationales section as to their existence but why they were excluded.)
Maybe this should be added to logical rules? (While you mention "tags values" specifically, I think "logical" may reasonably encompass rules like this too.)
- 'jsdoc/check-syntax'
- 'jsdoc/no-bad-blocks'
- 'jsdoc/imports-as-dependencies' (may not be ready due to an issue, but fits here I think)
(While 'jsdoc/check-examples' would fit with logical rules, as it is ESLint 7 only, I think we can indeed safely forget it.)
Should these be added to stylistic rules?
- 'jsdoc/check-indentation' (I can see this being omitted as some like to indent, but good to mention that omitted)
- 'jsdoc/check-line-alignment' (set to "never" sounds good)
- 'jsdoc/require-asterisk-prefix' (seems a reasonable default)
- 'jsdoc/require-hyphen-before-param-description' (set to "never" sounds good)
(I know 'jsdoc/sort-tags' is more dramatic, so reasonably disabled, but worth mentioning in a rationales section.)
Should these be added to requirements?
- 'jsdoc/require-template'
- 'jsdoc/require-throws'
('jsdoc/require-file-overview' and 'jsdoc/convert-to-jsdoc-comments' should I think be mentioned in a rationales section but not included as they are more dramatic.)
How about a name-and-description checking config for these?
- 'jsdoc/no-blank-blocks'
- 'jsdoc/informative-docs'
- 'jsdoc/match-description'
- 'jsdoc/match-name'
- 'jsdoc/no-blank-block-descriptions'
- 'jsdoc/require-description'
- 'jsdoc/require-description-complete-sentence'
- 'jsdoc/text-escaping'
Just to add mention under rationales of two uncategorized rules (uncategorized because they expect no defaults):
- 'jsdoc/no-missing-syntax'
- 'jsdoc/no-restricted-syntax'
I'm up for shuffling rule values around 🙌! Some clarifying questions...
+1 from me, I think they'd be great to add. Would this be a followup issue for the next major version to add them to recommended, too? Most rules I added as suggested, just a few questions on some...
I'm a bit torn over this one. It kind of is a stylistic rule, but also violations of it prevent logic from coming through... left to my own devices, I'd put it in stylistic to be honest. What do you think? I put it in the logical list for now.
😬 I personally really wouldn't want this rule in there. I've seen longer JSDoc comments use a indentation to convey some kind of meaning, and I've personally done that for lists too. I left it out for now - is that ok?
I, ah, have opinions on this topic 😅 [past article of mine discussing why TypeScript doesn't have a
I love it! Really pleased to see Just nitpicking, please forgive me: this'd be the only one with a >1 word specifier for the name. How about ... contents as a single word?
These are quite strict IMO. I personally wouldn't want to enable them, and I've seen other developers chafe pretty severely at them in the past. Especially for small functions (
That makes sense and sounds reasonable to me, but I'm not confident in writing the rationales myself. Could I lean on you to do that please? |
Sure.
I think this would be best kept at logical because it really impacts processing--at least the processing that our plugin does. If it is missing an asterisk, it doesn't get examined internally. Most of what we deal with may be technically stylistic from the viewpoint of JavaScript which ignores comments, but I think for our purposes, it is more logical.
Yeah, I agree.
Hehe. Sure, we can leave it out. I personally think something is better than nothing, but it indeed may be inherently incomplete.
Haha. It's already in the club of the round table of jsdoc plugin rules though. 😄
Sure. Good idea.
Sure.
Sure. I do plan to tack it on to this PR though so I can get your review on it. Let me know when you may be ready on your side for me to make additions. |
Great, thanks! It's ready now. |
96c44c0
to
57dae86
Compare
Ok, let me know what you think of the added commits (and I also rebased). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 this looks great to me, thank you!
Co-authored-by: Brett Zamir <brettz9@yahoo.com>
…ishability from bolding asterisks)
57dae86
to
8a998e9
Compare
🎉 This issue has been resolved in version 50.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Thanks for all your work on this! |
Fixes #1175.
#1175 (comment) was really helpful for tinkering with the rule names. I ended up splitting them into three sections:
Edit: per the review, there's now also:
In theory these could also be made for non-TypeScript and/or eslintrc, but ... I don't use those and am not as familiar with them. So I held off.