Skip to content
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

Merged
merged 1 commit into from
Jun 1, 2016
Merged

Add TomDoc support to no-deprecated rule #321

merged 1 commit into from
Jun 1, 2016

Conversation

josh
Copy link
Contributor

@josh josh commented May 8, 2016

Closes #302.

@josh
Copy link
Contributor Author

josh commented May 8, 2016

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?

@benmosher
Copy link
Member

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.

@josh
Copy link
Contributor Author

josh commented May 9, 2016

Ah, sorry. I forgot you mentioned that in the other thread. Yeah, I'll make
this modal.
On Mon, May 9, 2016 at 2:49 AM Ben Mosher notifications@github.com wrote:

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.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#321 (comment)

@benmosher
Copy link
Member

benmosher commented May 9, 2016

It probably needs to be a setting, at least for now, though (not a rule parameter--I misspoke above).

Parsing of imported modules can occur during evaluation of any rule, and only happens once, so the option would have to be exposed on context.settings for the ExportMap parse to know about it reliably.

Something like import/docstyle maybe? Actually, I summon @jfmengels to think of a better setting name? 😅 I'm not sure what to call the class of things that JSDoc/ESDoc/TomDoc specify. doctype? docstring-type?

Default value should probably be documented as jsdoc, though I think it matches ESDoc.

@jfmengels
Copy link
Collaborator

I summon @jfmengels to think of a better setting name

I always delegate that part to others myself :p, but I like docstyle ;)

I think it would be nice if this setting accepted an array of strings, like XX: ["tomdoc", "jsdoc"], in case you have multiple doc styles in your repo (while transitionning?), or simply no style (sometimes it's this, sometimes it's that...).

@@ -97,6 +97,14 @@ export default class ExportMap {
return m // can't continue
}

const docstyle = (context.settings && context.settings['import/docstyle']) || { jsdoc: true }
Copy link
Contributor Author

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?

Copy link
Member

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.

@josh
Copy link
Contributor Author

josh commented May 10, 2016

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

@benmosher
Copy link
Member

benmosher commented May 10, 2016

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. {jsdoc:false} is meaningless vs. just omitting) but it does allow potentially passing an object of options, later down the road (if it comes to that).

So I think it's probably good as-is, to start. Doesn't feel like it's worth publishing the contents of captureTomDoc to npm just to say that you can specify any arbitrary docparser.

So, yeah, a good first step towards general support for alternate metadata formats. 😄

@jfmengels
Copy link
Collaborator

jfmengels commented May 10, 2016

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.

@josh
Copy link
Contributor Author

josh commented May 10, 2016

I'm a little torn on specifying parsers as an object vs. just a list of strings.

👍 I'll make that change

@josh
Copy link
Contributor Author

josh commented May 22, 2016

Sorry, I think I forgot to comment that I updated the PR with @jfmengels "list of strings" suggestion.

@benmosher
Copy link
Member

Ah, sweet. LGTM

@benmosher
Copy link
Member

Actually can you document this new functionality in the rule's documentation and add a blurb to the change log?

@josh
Copy link
Contributor Author

josh commented May 23, 2016

Actually can you document this new functionality in the rule's documentation and add a blurb to the change log?

Done.

@benmosher
Copy link
Member

Can you document the import/docstyle setting as well? Probably just in the rule documentation for now, since nothing else references it.

See the main README for examples of existing setting documentation. I normally like to include a snippet of example .eslintrc config (in YAML). You don't need to follow suit, just offering as a reference.

Getting close. Sorry this has been dragging, I've been busy IRL. 😅

@josh
Copy link
Contributor Author

josh commented May 26, 2016

Can you document the import/docstyle setting as well? Probably just in the rule documentation for now, since nothing else references it.

Good call! I actually forgot it was opt in for a second.

Getting close. Sorry this has been dragging, I've been busy IRL. 😅

No worries! 😄 You've been giving great feedback.

@benmosher benmosher merged commit 23b9d12 into import-js:master Jun 1, 2016
@josh
Copy link
Contributor Author

josh commented Jun 1, 2016

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants