-
-
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
Standardize file names in packages/jest-worker and packages/pretty-format #7316
Conversation
CHANGELOG.md
Outdated
@@ -95,6 +95,8 @@ | |||
- `[docs]` Add correct default value for `testUrl` config option ([#7277](https://github.com/facebook/jest/pull/7277)) | |||
- `[jest-util]` [**BREAKING**] Remove long-deprecated globals for fake timers ([#7285](https://github.com/facebook/jest/pull/7285)) | |||
- `[docs]` Remove duplicate code in `MockFunctions` ([#7297](https://github.com/facebook/jest/pull/7297)) | |||
- `[jest-haste-map]` Standardize filenames ([#7316](https://github.com/facebook/jest/pull/7316)) |
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.
jest-haste-map -> jest-worker
@@ -9,7 +9,7 @@ | |||
|
|||
import type {Config, Printer, Refs} from 'types/PrettyFormat'; | |||
|
|||
import escapeHTML from './escape_html'; | |||
import escapeHTML from './escapeHTML'; |
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.
Can you rename the rest of the files in pretty-format? There's quite a bunch of left
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.
Hey, I might have missed some, could you point out what files are left? I thought that the ones in /src/
have multiple exports per file, hence I left files with snake_case
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 want to rename every file that doesn't stick to the convention.
E.g.
packages/pretty-format/src/__tests__/asymmetric_matcher.test.js
packages/pretty-format/src/plugins/asymmetric_matcher.js
to have names as variables:
import AsymmetricMatcher from './plugins/asymmetric_matcher';
import ConvertAnsi from './plugins/convert_ansi';
import DOMCollection from './plugins/dom_collection';
import DOMElement from './plugins/dom_element';
import Immutable from './plugins/immutable';
import ReactElement from './plugins/react_element';
import ReactTestComponent from './plugins/react_test_component';
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.
But the './plugins/asymmetric_matcher'
has 3 exports, so according to those:
All folders -> kabab-case
All files that export a single Class -> CapitalizeCase
All files that export a single function -> camelCase
All other files -> underscore_case
its falling into the underscore_case
?
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 is a mistake in interface, they shouldn't expose serialize
and test
directly, as they do it in a default export – and we only use it that way. You can remove those named exports. cc @pedrottimark
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.
@thymikee Great, I will redo the exports and fix those files, thanks 👍
@thymikee Hey, I've renamed plugins, but now I got quite a lot of test failures, related to the fact that I removed the exports for |
Oh, that's interesting. For now let's just bring back the exports if they're just for testing |
@thymikee 👍 reverted the mentioned exports and now everything is passing |
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.
👍
@@ -11,7 +11,7 @@ | |||
|
|||
import React from 'react'; | |||
import Immutable from 'immutable'; | |||
import {getPrettyPrint} from './expect_util'; |
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.
The getPrettyPrint
now only exports a single fn with the same name. Since there's no other exports there, maybe it's worth to change it to default export?
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.
Did the changes, not sure if that's what you meant, please take a look 👍
Thanks for sticking with me! |
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
This is part of #4969 - standardizes file names in
packages/jest-worker
andpackages/pretty-format
under Facebook naming conventionsTest plan
I've run yarn test and although its not passing, I'm getting same test failures on master