gajus / eslint-plugin-jsdoc

JSDoc specific linting rules for ESLint.
Other
1.1k stars 160 forks source link

feat: added granular flat TypeScript configs #1298

Closed JoshuaKGoldberg closed 3 months ago

JoshuaKGoldberg commented 3 months ago

Fixes #1175.

https://github.com/gajus/eslint-plugin-jsdoc/issues/1175#issuecomment-2272660984 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.

JoshuaKGoldberg commented 3 months ago

I'm up for shuffling rule values around 🙌! Some clarifying questions...

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.

+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...

jsdoc/no-bad-blocks

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.

jsdoc/check-indentation

😬 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?

jsdoc/require-throws

I, ah, have opinions on this topic 😅 [past article of mine discussing why TypeScript doesn't have a throws keyword]. IMO trying to enforce @throws or other what-errors-will-be-thrown annotations in JS/TS code is fundamentally always incomplete. I'm one of many (I think) who just avoid the practice altogether. Is it ok if we leave this out?

How about a name-and-description checking config for these?

I love it! Really pleased to see informative-docs getting promoted to a config. 😄

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?

jsdoc/require-description jsdoc/require-description-complete-sentence

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 (function getName() { return name; } where there's not much to add - this feels like it'd be in an even stricter variant of the requirements- ruleset. Is it ok if we leave them out?

mentioned in a rationales section

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?

brettz9 commented 3 months ago

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.

+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?

Sure.

Most rules I added as suggested, just a few questions on some...

jsdoc/no-bad-blocks

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 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.

jsdoc/check-indentation

😬 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?

Yeah, I agree.

jsdoc/require-throws

I, ah, have opinions on this topic 😅 [past article of mine discussing why TypeScript doesn't have a throws keyword]. IMO trying to enforce @throws or other what-errors-will-be-thrown annotations in JS/TS code is fundamentally always incomplete. I'm one of many (I think) who just avoid the practice altogether. Is it ok if we leave this out?

Hehe. Sure, we can leave it out. I personally think something is better than nothing, but it indeed may be inherently incomplete.

How about a name-and-description checking config for these?

I love it! Really pleased to see informative-docs getting promoted to a config. 😄

Haha. It's already in the club of the round table of jsdoc plugin rules though. 😄

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?

Sure. Good idea.

jsdoc/require-description jsdoc/require-description-complete-sentence

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 (function getName() { return name; } where there's not much to add - this feels like it'd be in an even stricter variant of the requirements- ruleset. Is it ok if we leave them out?

Sure.

match-name should also be removed if you added it already because I forgot this rule doesn't have any default behavior. Likewise with text-escaping which fails without an option and many do want Markdown contrary to that option it has.

mentioned in a rationales section

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 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.

JoshuaKGoldberg commented 3 months ago

Great, thanks! It's ready now.

brettz9 commented 3 months ago

Ok, let me know what you think of the added commits (and I also rebased).

github-actions[bot] commented 3 months ago

:tada: This issue has been resolved in version 50.2.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

brettz9 commented 3 months ago

Thanks for all your work on this!