Closed johnslemmer closed 5 years ago
Thanks for the PR @johnslemmer! (And thanks for reviewing @yxliang01!)
I'm a bit off the grid right now, so it will be a few days before I'll be able to fully review and merge.
I think this would be a good candidate to add as option which isn't enabled by default, like including Travis CI configuration:
https://github.com/bitjson/typescript-starter/blob/9bdf46aaf6f2853257eaf1d44d470c4438fd2467/src/cli/args.ts#L22 And here's the prompt: https://github.com/bitjson/typescript-starter/blob/9bdf46aaf6f2853257eaf1d44d470c4438fd2467/src/cli/inquire.ts#L112
Looks like we'll also need to add it to the integration tests, e.g. https://github.com/bitjson/typescript-starter/blob/9bdf46aaf6f2853257eaf1d44d470c4438fd2467/src/cli/tests/cli.integration.spec.ts#L508
Hey, so I'm definitely willing to make .editorconfig
optional. But first, I'm hoping I can persuade you to reconsider having it be a default and always-on sort of config.
First, a lot of projects use .editorconfig
. It is incredibly ubiquitous. I rummaged together this ridiculous GitHub search query of the .editorconfigs in the top 100 JavaScript/TypeScript repos. The total is 61 of 100. Not all but certainly a pretty incredible amount (I wanted to figure out how to do more repos with this search, but the GitHub search API doesn't make that easy).
Even projects that use Prettier still include it: https://github.com/facebook/react
Also, these changes are complementary to the prettier config that you already include by default with this utility.
Lastly, without .editorconfig
the developer experience using this utility leaves something to be desired (ie. #128 ).
Definitely respect your decision and will make the necessary changes for whatever you decide. Thanks.
PS I updated the tests on this PR to pass.
Hey @johnslemmer, thanks again for the PR. I'm back on the grid now – sorry for the long delay here.
Thanks for the through rationale! I'm convinced – I'd be happy with enabling it by default. 👍
I do want to avoid the scenario where users who don't want it end up manually deleting .editorconfig
after the CLI finishes though (if only to avoid the feeling that the output is "bloated").
Would you be willing to add it as an enabled-by-default "extra" in this PR? Much like Circle CI is here: https://github.com/bitjson/typescript-starter/blob/bd7f021b4fb84c0e7e560ce701c93683f05fc2bd/src/cli/inquire.ts#L101-L105 and here: https://github.com/bitjson/typescript-starter/blob/bd7f021b4fb84c0e7e560ce701c93683f05fc2bd/src/cli/args.ts#L39-L42
Then if the user disables it, it can just be deleted like other similar options: https://github.com/bitjson/typescript-starter/blob/bd7f021b4fb84c0e7e560ce701c93683f05fc2bd/src/cli/typescript-starter.ts#L147-L149
Merging #131 into master will not change coverage. The diff coverage is
100%
.
@@ Coverage Diff @@
## master #131 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 9 9
Lines 256 259 +3
Branches 38 39 +1
=====================================
+ Hits 256 259 +3
Impacted Files | Coverage Δ | |
---|---|---|
src/cli/utils.ts | 100% <ø> (ø) |
:arrow_up: |
src/cli/tasks.ts | 100% <ø> (ø) |
:arrow_up: |
src/cli/args.ts | 100% <ø> (ø) |
:arrow_up: |
src/cli/inquire.ts | 100% <100%> (ø) |
:arrow_up: |
src/cli/typescript-starter.ts | 100% <100%> (ø) |
:arrow_up: |
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 56cf111...7df5f04. Read the comment docs.
@bitjson let me know if these most recent changes work.
@johnslemmer fantastic, thanks!
add an .editorconfig to keep editors behaving as you would expect (especially on new files) before prettier has had a chance to format the file. This was mostly copied from the React JS project, and similar to what is seen in many others.
Addresses #128