-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add TomDoc support to no-deprecated rule #321
Conversation
Re: #302 (comment) @benmosher what do you think? Does it feel too ad-hoc to have a mini tomdoc parser as part of this repo? |
Looks good, in theory. I am a little concerned about spurious flagging, though. I think I'd like to have a "style" parameter that defaults to JSDoc but can allow specification of TomDoc. |
Ah, sorry. I forgot you mentioned that in the other thread. Yeah, I'll make
|
It probably needs to be a Parsing of imported modules can occur during evaluation of any rule, and only happens once, so the option would have to be exposed on Something like Default value should probably be documented as |
I always delegate that part to others myself :p, but I like I think it would be nice if this setting accepted an array of strings, like |
@@ -97,6 +97,14 @@ export default class ExportMap { | |||
return m // can't continue | |||
} | |||
|
|||
const docstyle = (context.settings && context.settings['import/docstyle']) || { jsdoc: true } |
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.
Is there a better place to define plugin default settings
so it gets merged correctly with any user config?
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.
Not at the moment, nope. I've been doing it as-hoc as you've done here.
@benmosher what do you think so far? The integration still feels a little gross, but I don't think I understand enough of the requirements for building a useful abstraction that will allow other doc parsers to hook in. |
I think it looks good. I'm a little torn on specifying parsers as an object vs. just a list of strings. I can't think of any reason one would explicitly disable one (i.e. So I think it's probably good as-is, to start. Doesn't feel like it's worth publishing the contents of So, yeah, a good first step towards general support for alternate metadata formats. 😄 |
I agree with @benmosher, I'd also prefer a list of strings, would make the settings a bit shorter and more readable in the config. |
👍 I'll make that change |
Sorry, I think I forgot to comment that I updated the PR with @jfmengels "list of strings" suggestion. |
Ah, sweet. LGTM |
Actually can you document this new functionality in the rule's documentation and add a blurb to the change log? |
Done. |
Can you document the See the main README for examples of existing setting documentation. I normally like to include a snippet of example Getting close. Sorry this has been dragging, I've been busy IRL. 😅 |
Good call! I actually forgot it was opt in for a second.
No worries! 😄 You've been giving great feedback. |
Thanks! |
Closes #302.