Skip to content

New rule: Check param existence#138

Closed
jdlrobson wants to merge 1 commit intojscs-dev:masterfrom
jdlrobson:checkParamExistence
Closed

New rule: Check param existence#138
jdlrobson wants to merge 1 commit intojscs-dev:masterfrom
jdlrobson:checkParamExistence

Conversation

@jdlrobson
Copy link
Copy Markdown
Contributor

This rule reports errors when a parameter is undocumented or missing
an @inheritdoc statement

If @inheritdoc is present it assumes that the function inherited from is documented correctly.
This seems useful for ensuring a developer has documented everything they should have in a function signature.

This patch is not quite mergeable - I need to write some tests but would you welcome a patch for this?

@jdlrobson
Copy link
Copy Markdown
Contributor Author

(As stated I need to work on this a bit more, but I figured I'd send a pull request to identify whether any other teams would benefit from this before investing the effort :-))

We've been using it for some time and it has proved handy.

@qfox
Copy link
Copy Markdown
Member

qfox commented Aug 4, 2015

Thanks for contribution. Would be nice to have this in checker. ;-)

👍 from me

@jdlrobson
Copy link
Copy Markdown
Contributor Author

Does checkParamExistence make sense as a rule name or is there a better name I should use? I will work to get this in mergeable form :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be another check like !node.jsdoc.valid. But I'm not sure we really need this check here since this rule for function scope ;-)

@qfox
Copy link
Copy Markdown
Member

qfox commented Aug 5, 2015

There is a huge conflicting rule checkParamNames. Can you take a look at it? Looks like it should skip checking if there are @inheritdoc or some other tags you use here.

@jdlrobson
Copy link
Copy Markdown
Contributor Author

Should I merge checkParamNames with checkParamExistence or is it better to have them separate? I can't think of a reason where you wouldn't want to check both..

@qfox
Copy link
Copy Markdown
Member

qfox commented Aug 6, 2015

Not sure. checkParamNames already weighs a lot. I'd go to ignore this logic somehow in checkParamNames (just to prevent false reports), and left checkParamExistence as is. But you're right so guess better to merge them into one rule.

@piotr-cz
Copy link
Copy Markdown

What is the reason of not supporting @inheritdoc in the jsdoc3 annotation type, when it's available in jsduck5 annotation type?

@qfox
Copy link
Copy Markdown
Member

qfox commented Aug 19, 2015

@jdlrobson
Copy link
Copy Markdown
Contributor Author

Apologies this is taking me so long. I tried recently but am a bit rusty in this repository and couldn't get npm test to run in my local dev environment and didn't have the time to debug. I will try again as soon as I have a moment.

@qfox
Copy link
Copy Markdown
Member

qfox commented Aug 19, 2015

Personally I'll try to find some time next weekend to merge all PRs, but there is another strange issue with bundled to jscs version. I'm not sure how much time it takes.

But if you have some troubles with npm test — just tell me, I'm almost always ready to help ;-) Btw, there is a chat at gitter.im: https://gitter.im/jscs-dev/node-jscs

@jdlrobson jdlrobson force-pushed the checkParamExistence branch 3 times, most recently from 2c597d7 to ad80271 Compare August 21, 2015 23:25
@jdlrobson
Copy link
Copy Markdown
Contributor Author

okay pushed. I wasn't able to see the issues with checkParamNames option but these rules might want to be merged in future. I've given you loads of test cases so hopefully that will give you confidence in my patch! It's good to be contributing again! :)

@jdlrobson jdlrobson force-pushed the checkParamExistence branch from ad80271 to 393ffc6 Compare August 26, 2015 15:54
This rule reports errors when a parameter is undocumented or missing
an @inheritdoc statement
@qfox qfox closed this in aa40ed5 Sep 6, 2015
wmfgerrit pushed a commit to wikimedia/mediawiki-extensions-MobileFrontend that referenced this pull request Sep 11, 2015
Our upstream patch got merged: jscs-dev/jscs-jsdoc#138
Fix some issues with bad use of @return parameter

Change-Id: Ifad8bcc99508490192efc4a1659c8c814fbdb3f3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants