[jest-validate] Add config comments feature #7295

andrew-pyle · 2018-10-28T22:14:51Z

Summary

Add feature requested in #6027. Support comments in package.json.

The NPM-sanctioned method for adding comments to package.json is via a "//" key with the comment as the value. This is done to preserve package.json as a strict JSON file. See the following:

Adds "//" to the default blacklist in packages/jest-validate/src/default_config.js.

API reference in docs/Configuration.md updated with a bit in the introduction and an entry for "//" in the Reference section.

Test plan

Unit Test

test('Comments using "//" key are not warned by default') added to __tests__/validate.test.js and passes. The test was based on the test('respects blacklist') test

Test Suites

Output of yarn run test was the same both before the changes and after, with the exception of the added unit test, which passes.

See test-suite-output.txt

Possible Issue

Passing a recursiveBlacklist array in the ValidationOptions overwrites the defaults in default_config.js and restores the warning for "//" keys.

Is this a desirable functionality, or should the "//" key be permanently included in the recursiveBlacklist array passed to the validate() function?

thymikee

LGTM

thymikee · 2018-10-28T22:28:54Z

docs/Configuration.md

+    "//": "Comment goes here",
+    "verbose": true
+  }
+}


I don't think we need to document this, but I won't complain

So leave as is?

Delete here and leave the one below

SimenB

Great job!

Possible Issue

Passing a recursiveBlacklist array in the ValidationOptions overwrites the defaults in default_config.js and restores the warning for "//" keys.
Is this a desirable functionality, or should the "//" key be permanently included in the recursiveBlacklist array passed to the validate() function?

I think we should ensure that // is allowed even on custom blacklists

packages/jest-validate/src/__tests__/validate.test.js

SimenB · 2018-10-28T22:39:23Z

You can run yarn prettylint docs/Configuration.md --fix locally to fix the error reported by CI 🙂

SimenB · 2018-10-28T22:39:55Z

Oh, can you add an entry to the changelog for this?

andrew-pyle · 2018-10-28T23:32:23Z

Useful but not completely necessary question:

I am able to use the tests in __tests__ to test my code changes, but I am having trouble setting up a new project to use my development build of Jest as documented in CONTRIBUTING.md.

I'd like to manually run Jest to see my changes in action.

Would linking jest-cli with yarn be enough to use my updated jest-validate code? Or do I need to link the jest-validate code specifically? Could someone make a suggestion?

codecov-io · 2018-10-29T00:10:45Z

Codecov Report

Merging #7295 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #7295      +/-   ##
==========================================
+ Coverage   66.54%   66.54%   +<.01%     
==========================================
  Files         241      241              
  Lines        9316     9317       +1     
  Branches        6        6              
==========================================
+ Hits         6199     6200       +1     
  Misses       3114     3114              
  Partials        3        3

Impacted Files	Coverage Δ
packages/jest-validate/src/validate.js	`100% <100%> (ø)`	⬆️
packages/jest-jasmine2/src/jasmineAsyncInstall.js
packages/jest-jasmine2/src/jasmine/createSpy.js
packages/jest-jasmine2/src/jasmine/SpyStrategy.js
packages/jest-jasmine2/src/treeProcessor.js
...ackages/jest-jasmine2/src/assertionErrorMessage.js
packages/jest-jasmine2/src/isError.js
packages/jest-jasmine2/src/jestExpect.js
packages/jest-jasmine2/src/jasmine/CallTracker.js
packages/jest-jasmine2/src/errorOnPrivate.js
... and 25 more

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 5a624d4...81c287f. Read the comment docs.

rickhanlonii · 2018-10-29T03:02:33Z

@ndrew-pyle yeah you should be able to link as it's documented in the CONTRIBUTING.md - what are you seeing that doesn't work

Also - do we need to document this? I don't really think it's necessary to call it out since it's not a Jest thing. The few people that know about it will use it and those who don't won't need us to tell them about it

SimenB · 2018-10-29T07:26:57Z

Might make sense to document in https://github.com/facebook/jest/blob/master/packages/jest-validate/README.md

andrew-pyle · 2018-10-29T15:02:25Z

@rickhanlonii I'm new to Jest, so I'm attributing this to my inexperience.

Even though the tests for my changes in the jest-validate repository are passing as expected, when I run my linked Jest in a local test repo, I'm still getting the Validation warning. Same as on https://repl.it/repls/ThornyHatefulVirus. This could mean that there is still an issue with the code, but I'm not sure how to verify that my code changes are actually being used.

EDIT:

The changes seem to be effective in my test repo now. I'm not sure what changed, but I feel that the cause was indeed inexperience.

- Supplies empty array when user-supplied ValidationOptions has no recursiveBlacklist key.

andrew-pyle · 2018-10-29T17:07:12Z

@SimenB Made change to include the default blacklist even with user-supplied blacklists in 81c287f.

Concatenates the recursiveBlacklist array from default_config.js with the ValidationOptions object passed as an argument
Uses || boolean to supply empty array if ValidationOptions doesn't have a recursiveBlacklist key.

Potential Issue
All items in the recursiveBlacklist array from default_config.js are preserved, meaning that if more elements are added in addition to "//", they will also be permanent from the user's perspective. Is this the preferred functionality?

thymikee · 2018-10-29T22:29:20Z

Should be fine :). Can you rebase/merge latest changes from master?

SimenB · 2018-10-30T14:05:35Z

Thanks @andrew-pyle!

andrew-pyle · 2018-10-30T16:27:50Z

@thymikee @SimenB @rickhanlonii Thank you all! This has been a pleasant first contribution to an open source project!

rickhanlonii · 2018-10-31T14:39:12Z

Thank you @andrew-pyle and congrats on your first Jest contribution 😃

github-actions · 2021-05-12T06:07:27Z

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.

Andrew Pyle and others added 3 commits October 27, 2018 14:04

Add '//' to default jest configuration blacklist & add test

96f4ba2

Add note to docs about using "//" keyed comments in package.json

115af2d

Add API reference for "//"

498a225

facebook-github-bot added the cla signed label Oct 28, 2018

thymikee approved these changes Oct 28, 2018

View reviewed changes

andrew-pyle mentioned this pull request Oct 28, 2018

Don't print Validation Warning for "//" comments in package.json #6027

Closed

SimenB reviewed Oct 28, 2018

View reviewed changes

packages/jest-validate/src/__tests__/validate.test.js Outdated Show resolved Hide resolved

Remove "//" documentation from text introduction per @thymikee's request

4a5c62b

andrew-pyle added 3 commits October 28, 2018 18:33

Run prettylint on Configuration.md to fix CI errors

5f519cc

Add entry to CHANGELOG.md

986f655

Don't overwrite console.warn manually per @SimenB's request

600944b

andrew-pyle added 3 commits October 29, 2018 10:53

Keep defaults "//" in blacklist even with user supplied blacklists.

3e533d8

Add boolean to catch undefined user-supplied blacklist

2a56e04

- Supplies empty array when user-supplied ValidationOptions has no recursiveBlacklist key.

Prevent undefined errors when combining blacklists

81c287f

thymikee requested review from SimenB and rickhanlonii October 29, 2018 22:28

SimenB approved these changes Oct 30, 2018

View reviewed changes

Merge branch 'master' into add-config-comments-feature

c39da98

SimenB merged commit fb61bff into jestjs:master Oct 30, 2018

andrew-pyle changed the title ~~[WIP] [jest-validate] Add config comments feature~~ [jest-validate] Add config comments feature Oct 30, 2018

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[jest-validate] Add config comments feature #7295

[jest-validate] Add config comments feature #7295

andrew-pyle commented Oct 28, 2018

thymikee left a comment

thymikee Oct 28, 2018

andrew-pyle Oct 28, 2018

thymikee Oct 28, 2018

SimenB left a comment

SimenB commented Oct 28, 2018 •

edited

Loading

SimenB commented Oct 28, 2018

andrew-pyle commented Oct 28, 2018

codecov-io commented Oct 29, 2018 •

edited

Loading

rickhanlonii commented Oct 29, 2018

SimenB commented Oct 29, 2018

andrew-pyle commented Oct 29, 2018 •

edited

Loading

andrew-pyle commented Oct 29, 2018 •

edited

Loading

thymikee commented Oct 29, 2018

SimenB commented Oct 30, 2018

andrew-pyle commented Oct 30, 2018

rickhanlonii commented Oct 31, 2018

github-actions bot commented May 12, 2021

[jest-validate] Add config comments feature #7295

[jest-validate] Add config comments feature #7295

Conversation

andrew-pyle commented Oct 28, 2018

Summary

Test plan

Unit Test

Test Suites

Possible Issue

thymikee left a comment

Choose a reason for hiding this comment

thymikee Oct 28, 2018

Choose a reason for hiding this comment

andrew-pyle Oct 28, 2018

Choose a reason for hiding this comment

thymikee Oct 28, 2018

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

Possible Issue

SimenB commented Oct 28, 2018 • edited Loading

SimenB commented Oct 28, 2018

andrew-pyle commented Oct 28, 2018

codecov-io commented Oct 29, 2018 • edited Loading

Codecov Report

rickhanlonii commented Oct 29, 2018

SimenB commented Oct 29, 2018

andrew-pyle commented Oct 29, 2018 • edited Loading

andrew-pyle commented Oct 29, 2018 • edited Loading

thymikee commented Oct 29, 2018

SimenB commented Oct 30, 2018

andrew-pyle commented Oct 30, 2018

rickhanlonii commented Oct 31, 2018

github-actions bot commented May 12, 2021

SimenB commented Oct 28, 2018 •

edited

Loading

codecov-io commented Oct 29, 2018 •

edited

Loading

andrew-pyle commented Oct 29, 2018 •

edited

Loading

andrew-pyle commented Oct 29, 2018 •

edited

Loading