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

Standardize file naming #4969

Closed
mjesun opened this issue Nov 27, 2017 · 61 comments · Fixed by #5298
Closed

Standardize file naming #4969

mjesun opened this issue Nov 27, 2017 · 61 comments · Fixed by #5298

Comments

@mjesun
Copy link
Contributor

mjesun commented Nov 27, 2017

File naming conventions in Jest can be improved. A good example is the integration_tests folder. We have:

  • Dashed names: auto-clear-mocks
  • Underscored names: clear_cache
  • Camel case names: testNamePattern
  • Dash + camel case names: timer-resetMocks
  • Underscore + camel case names: stack_trace_no_captureStackTrace
  • ? names: regex-(char-in-path

I understand that some of the camel cased ones reference actual method names, like captureStackTrace, but I think we could do it better. This is absolutely not top-priority, but I just wanted to add a task to it because it feels there's room for improvement.

@cpojer
Copy link
Member

cpojer commented Nov 27, 2017

I want to use camelCase for functions, dashes for folders and Capitalized for classes. I'll send a tshirt to the person that makes this happen.

@thymikee
Copy link
Collaborator

cc @aaronabramov

@mjesun
Copy link
Contributor Author

mjesun commented Nov 27, 2017

+1 to @cpojer's suggestion. I think it matches the "standard" at the industry.

@thymikee
Copy link
Collaborator

For the reference: #3771

@slootzky
Copy link

I'll take a look on this

@ajomadlabs
Copy link

@mjesun Can I take up this issue

@mjesun
Copy link
Contributor Author

mjesun commented Dec 5, 2017

@ajomadlabs maybe you can sync with @slootzky? There are a lot of files, so you can maybe split; but I'd rather double-check before getting the work done twice 🙂

@slootzky
Copy link

slootzky commented Dec 7, 2017

@ajomadlabs @mjesun sorry for not being responsive , well I actually did all the necessary changes (I believe) but then had to work on something else and didn't have the chance to start testing all.
I really hope to do it tomorrow and then I'll send the PR.

@ajomadlabs, if I see I'm still not getting to this tomorrow, I will update here.

@slootzky
Copy link

Hey @ajomadlabs @mjesun ,
Sorry but I just can't get to completing this :(
Guess I will just need to find some time and get another issue fixed.

@ajomadlabs
Copy link

@slootzky so can I take up this

@mjesun
Copy link
Contributor Author

mjesun commented Dec 22, 2017

Thanks @ajomadlabs! Let me know if you need any help with something 🙂

@haroldtreen
Copy link

haroldtreen commented Dec 30, 2017

Is this issue sufficiently spec'ed out? I'm unclear.

The above comment suggest to use camelCase and CapitalizeCase, but the linked issues suggest everything be underscore case...

So is the final recommendation:

  • 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
  • Folders surrounded by __ (eg. __tests__) -> Stay the same.

This might be something to document in the contributing guidelines under Code Conventions.

@mjesun
Copy link
Contributor Author

mjesun commented Dec 30, 2017

Yes; how should we name things has always been a complex topic in Jest. I'm sorry if we created more confusion linking the other task, but I think it was necessary to keep proper track of everything 🙂

I think @cpojer's comment regarding PascalCase and camelCase was specifically referred to syntax rather than to files; but let's double-check. Personally, I would name all folders and files with kebab-case (except special folders, like __tests__ or __mocks__), so we do not make a distinction between files that export a single thing, or multiple ones.

I 100% agree with your suggestion of adding this to the code conventions; since we're the first ones (at Facebook) to consistently break them. Let's fully clarify here before proceeding.

@cpojer
Copy link
Member

cpojer commented Jan 3, 2018

Hey everyone! I’d love to stop bikeshedding about this and just revert to standard Facebook practice:

  • Files that primarily export types, objects or classes should use CapitalizedFileNames.js and should mirror what’s inside 1:1.
  • Files that export a single function should have the function name with camelCase in it.
  • Folder names should use dashes, unless they are special folders.

Let me know if anyone has any other questions for this one but I don’t think it’s ambiguous :)

@jamischarles
Copy link
Contributor

I’ll take a stab at this tomorrow. What does “done” look like? Tests passing as before? Anything else?

@thymikee
Copy link
Collaborator

@jamischarles yarn test should pass, which includes Flow and ESLint checks.

  • ESLint config should be set up to support new file naming.
  • Flow should be a great helper for this task, so make sure you have IDE support installed.

Also, maybe let's do it incrementally package-by-package (or a few if they're small), as this affects whole codebase and will basically make every existing PR affected.

@SimenB
Copy link
Member

SimenB commented Jan 12, 2018

We have an eslint rule for filename, is it possible to maintain that with different styles of names?

https://github.com/facebook/jest/blob/19376c1e1e8a77717fbb7a216df92087eddbd277/.eslintrc.js#L171

@jamischarles
Copy link
Contributor

@thymikee Makes sense, thanks.
@SimenB I suggest that during the transition we change this from error (2) to warning (1), and after we're done we change it back. How does that sound?

@SimenB
Copy link
Member

SimenB commented Jan 13, 2018

SGTM

@jamischarles
Copy link
Contributor

jamischarles commented Jan 13, 2018

Here's the first big PR for this. I did this one manually, just to get a feel for it.

Here's a check list to keep track of what still needs to be converted:

@SimenB
Copy link
Member

SimenB commented Jan 14, 2018

Only one done

@SimenB SimenB reopened this Jan 14, 2018
@atiqueansari1987
Copy link

Anything left to do in this issue?

@jamischarles
Copy link
Contributor

@atiqueansari1987 yes there is quite a bit of work left. You could either start with the task to rename file names under integration-tests or you can choose another folder. See the checklist above. Quite a bit of work left.

@jamischarles
Copy link
Contributor

I suggest renaming folders as a separate task / PR than renaming files. Just renaming the folders was enough work that it warranted its own PR. Up to you though.

@atiqueansari1987
Copy link

Thanks for the info. I will try to submit a PR this weekend.

@binygal
Copy link
Contributor

binygal commented Jan 25, 2018

I would love to help with that. If there is still work left to do, can someone point me to a place where work should be done?

@matmalkowski
Copy link
Contributor

I will pick up jest-worker and pretty-format

thymikee pushed a commit that referenced this issue Nov 1, 2018
<!-- Thanks for submitting a pull request! Please provide enough information so that others can review your pull request. The two fields below are mandatory. -->

<!-- Please remember to update CHANGELOG.md in the root of the project if you have not done so. -->

## Summary

This is part of #4969.
It standardizes file names in packages/jest-watcher under Facebook naming conventions.

## Test plan

Running `yarn run clean-all` followed by `yarn` and `yarn test` is not completely clean on my machine, but it yields the same results in this branch and `master`
@thymikee
Copy link
Collaborator

@betalantz are you still interested in converting files assigned to you?

screenshot 2018-11-14 at 10 30 52

@andrearosr need any help with jest-watcher? :)

@betalantz
Copy link
Contributor

Yes, I'm available to work on this. I'll follow the assignments above, unless they've been assigned elsewhere.

@GGonryun
Copy link
Contributor

GGonryun commented Dec 4, 2018

Hello! 🖐 First-time-contributor here. If there are any packages or folders remaining I can help take on a few.

@thymikee
Copy link
Collaborator

thymikee commented Dec 4, 2018

@GGonryun feel free to take over jest-watcher :)
And maybe root filenames in other PR?

@andrearosr
Copy link
Contributor

Hey there! I hadn't seen the other messages to this thread, I did jest-watcher in #7315 so I think the packages folder may be done

@hedgerh
Copy link

hedgerh commented Mar 30, 2019

picked up jest-snapshot. getting a PR ready. jest-runner needs to be checked off, as well. are there actually any root file/folders left to be renamed?

@AndresContreras96
Copy link

Hello, I also want to make my first contribution can I work on this issue?

@GabeNedden
Copy link

Hi, first-timer here! If there's room for another I'd love the opportunity to work on this issue. Please let me know.
Thank you!

@github-actions
Copy link

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Feb 26, 2022
@github-actions
Copy link

This issue was closed because it has been stalled for 7 days with no activity. Please open a new issue if the issue is still relevant, linking to this one.

@github-actions
Copy link

This issue 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 Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.