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

feat(jest-docblock): support multiple of the same @pragma #5502

Merged
merged 3 commits into from
Feb 9, 2018

Conversation

azz
Copy link
Contributor

@azz azz commented Feb 9, 2018

Summary

Currently, when jest-docblock parses:

/**
 * @foo x
 * @foo y
 */

we get { foo: 'y' }.

This PR changes it to { foo: ['x', 'y'] } when there are multiple.

The PR also adds support for that in docblock.print().

Test plan

Unit tests added and passing.

cc @vjeux

@azz azz force-pushed the docblock-multi-pragma branch from 3fad3f7 to e58ac7f Compare February 9, 2018 08:46
@codecov-io
Copy link

Codecov Report

Merging #5502 into master will increase coverage by 0.03%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5502      +/-   ##
==========================================
+ Coverage   61.67%   61.71%   +0.03%     
==========================================
  Files         213      213              
  Lines        7071     7075       +4     
  Branches        3        3              
==========================================
+ Hits         4361     4366       +5     
+ Misses       2709     2708       -1     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-haste-map/src/worker.js 96% <100%> (+0.16%) ⬆️
packages/jest-docblock/src/index.js 53.84% <25%> (+10.36%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3942361...104fc53. Read the comment docs.

@cpojer cpojer merged commit f2ef92d into jestjs:master Feb 9, 2018
@cpojer
Copy link
Member

cpojer commented Feb 9, 2018

Awesome, thanks for the fix. Internally at FB we discussed making this throw instead and I think for our internal use cases this makes sense, however I do not think that in open source Jest we can make such a decision. It's easier to support multiple of the same directives without having to ask users to change their behavior.

@azz azz deleted the docblock-multi-pragma branch February 9, 2018 11:26
@azz
Copy link
Contributor Author

azz commented Feb 9, 2018

Yeah, I can easily imagine this breaking TypeScript JSDoc, e.g:

/**
 * @typedef {Object} SpecialType - creates a new type named 'SpecialType'
 * @property {string} prop1 - a string property of SpecialType
 * @property {number} prop2 - a number property of SpecialType
 * @property {number=} prop3 - an optional number property of SpecialType
 * @prop {number} [prop4] - an optional number property of SpecialType
 * @prop {number} [prop5=42] - an optional number property of SpecialType with default value
 */

@vjeux
Copy link
Contributor

vjeux commented Feb 9, 2018

Thanks for doing this!

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants