-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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 validation wrapper for PatternPrompt class #7447
Conversation
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
@SimenB Can you please check the algorithm of my fix? |
@SerhiiBilyk Are you still working on this? I'd still love this feature to be included. I keep fat fingering things in watch mode. |
@natealcedo, I am waiting for review now |
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.
Code LGTM, thanks! This is however missing tests and a changelog entry before we can merge 🙂
Also, could you post some screenshots for how this looks?
@SimenB @rickhanlonii @rogeliog screenshots: |
@rickhanlonii @SimenB Can you please help me with testing? |
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.
Please update the changelog.
When it comes to a test, you can add an integration test in https://github.com/facebook/jest/blob/64b3a9b72b8cba4bd303cc0c9f87d23645635936/e2e/__tests__/watchModePatterns.test.js
There you can see how patterns are provided 🙂 If you're willing, some unit tests would be great as well!
@SerhiiBilyk I would suggest just writing all the tests first and then asking for a review. If you are unsure on how to write tests you should take a look at the CONTRIBUTING.md. That way, reviewers can give feedback on all of your code at one go rather than waiting for a review before writing tests. |
@natealcedo ok. |
@SimenB @rickhanlonii |
Yes, and in |
@jeysal thank you for your tip. Can you check please my unit test? |
btw, @SerhiiBilyk can you rebase your work to master? |
Don't worry too much about the conflict, we can resolve that ourselves before merging 🙂 |
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.
LGTM if CI passes
Edit: and if you add a changelog entry as mentioned in @SimenB's review :)
@@ -90,6 +100,16 @@ Object { | |||
} | |||
`; | |||
|
|||
exports[`Watch mode flows Pressing "P" enters pattern mode and invalid regexp 1`] = ` |
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.
This snapshot seems obsolete?
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.
Yes
@@ -39,7 +39,7 @@ export default class Prompt { | |||
|
|||
enter( | |||
onChange: (pattern: string, options: ScrollOptions) => void, | |||
onSuccess: () => void, | |||
onSuccess: (pattern: string) => void, | |||
onCancel: () => void, |
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.
onCancel
has different fn signature than https://github.com/facebook/jest/pull/7447/files?w=1#diff-048f6a2e1f2d542739b5b37c81a7e9c3R16, I think it's still should be _onCancel: (value?: string) => void
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.
We initialize to empty string, so the correct type is that it's passed. right?
Is it still relevant ? |
Yeah, I'd say so! Would you mind rebasing? |
I agree, this is still relevant |
* removed default value of prettierPath from argv * updated snapshot for show_config * updated changelog * fixed wrong package name in changelog
* (fix) message should return a function message should return a function and not a constant * Update CHANGELOG.md
* Update CHANGELOG formatting * Dedupe
* Migrate jest-changed-files to Typescript * Add changelog entry * Update e2e/__tests__/jestChangedFiles.test.js Co-Authored-By: loryman <[email protected]> * Stricter typings Merge remote-tracking branch 'origin/jest-changed-files-typescript' into jest-changed-files-typescript * Make prettier happy
## Summary It doesn't make sense to wrap a `runJust` call into `stripAnsi` since the latter works only with strings. `runJest` returns an object so stripAnsi just passes it along. Materials: - Commit where the code was added in the first place: jestjs@e12cab6#diff-4cf7d6c79ff377b63522b3af20e34e8eR307 - stripAnsi source code: https://github.com/chalk/strip-ansi/blob/097894423fedb6b4dca3005ad45608b893fcdcf8/index.js
* chore: migrate jest-matcher-util to TypeScript * link to PR * unknown and fix flow
59a33aa
to
f459c7a
Compare
This PR is stale because it has been open 1 year with no activity. Remove stale label or comment or this will be closed in 30 days. |
This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one. |
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. |
Summary
Due to #7382
Test plan