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

feat: generate tslint.json #160

Merged
merged 3 commits into from Jun 3, 2018
Merged

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Jun 1, 2018

init now generates a tslint.json file which inherits from the default
configuration. This empowers the users to customize if necessary
instead of being overly opinionated.

Fixes: #127
Fixes: #119

This depends upon #158 and #159, so the first two commits don't need to be reviewed. They will be removed once those PRs land.

@codecov-io
Copy link

codecov-io commented Jun 1, 2018

Codecov Report

Merging #160 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #160      +/-   ##
==========================================
+ Coverage   96.75%   96.77%   +0.01%     
==========================================
  Files          10       10              
  Lines         339      341       +2     
  Branches       24       24              
==========================================
+ Hits          328      330       +2     
  Misses         11       11
Impacted Files Coverage Δ
test/test-kitchen.ts 100% <100%> (ø) ⬆️

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 cef0d68...d19cef3. Read the comment docs.

@JustinBeckwith
Copy link
Collaborator

JustinBeckwith commented Jun 1, 2018

I am 👎 on this approach. My hope would be that the average user wouldn't need to have their own tslint.json. I'm mostly down on supporting the override at all, but I get why some people will just insist on it. Instead of generating this during init, could we add a command that drops the file for you? Like a gts customize-lint or something like that?

@ofrobots
Copy link
Contributor Author

ofrobots commented Jun 1, 2018

This is a good discussion.

This is what the README says about this project:

No configuration. The easiest way to enforce consistent style in your project. Just drop it in.

This also speaks to what you are trying to argue about the average user not needing to customize anything. I agree with that. Yet, this project, today, generates a tsconfig.json in the users's directory. Is that an invitation for the user to customize it? Maybe, maybe not (IMHO I don't think it is an invitation to customize). It is just how we wire things up. At the same time, we don't want this project to be too paternalistic and prevent people from customizing tsconfig.json. By the same argument, I do not think that generating a tslint.json is an invitation for the user to customize the lint config.

Either way, we need to be consistent in our own reason. We need to have a good reason why it is okay to generate tsconfig.json and not the tslint.json. We should generate both, or neither.

@JustinBeckwith
Copy link
Collaborator

Fair points! I suppose in a perfect world, I'd prefer we take the same approach with tsconfig.json. As long as there was an easy way to generate it, not needing to have that either would be great :)

`init` now generates a tslint.json file which inherits from the default
configuration. This empowers the users to customize if necessary
instead of being overly opinionated.

Fixes: google#127
Fixes: google#119
@@ -43,8 +43,6 @@ When you run the `npx gts init` command, it's going to do a few things for you:
- `compile`: Compiles the source code using TypeScript compiler.
- `pretest`, `posttest` and `prepare`: convenience integrations.

We strongly recommend you use the default style config, but if you must tweak, you can edit the generated `tsconfig.json`. For linter, we use the default `tslint.json` unless we find that file in your project directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentionally removed. If we are going to generate tslint.json – we do not need to document further. Let's not invite customization.

@ofrobots
Copy link
Contributor Author

ofrobots commented Jun 1, 2018

I think generating the files is friendlier. The value of this project is in providing a good, consistent style guide that can be used in a project without thinking about it.

If someone has thought about the precise, but different, style they want, and still find value in this project – let us not turn them away.

@ofrobots
Copy link
Contributor Author

ofrobots commented Jun 1, 2018

As long as there was an easy way to generate it, not needing to have that either would be great :)

I do not think this is practical. The user needs to be able to customize their repo structure.

@ofrobots
Copy link
Contributor Author

ofrobots commented Jun 2, 2018

The other reason why this is desirable is mentioned in #119: IDE support. A tslint.json in the correct location ends up getting picked up by the IDE or other tooling the user may have,.

I'm -1 on generating the configuration only under a flag, if provided by the user. That is not going to be discoverable. This needs to be the default behavior so that users don't have to pass a flag effectively equivalent to --and-work-with-my-IDE. I feel strong about this at this point.

@JustinBeckwith
Copy link
Collaborator

Again, all fair points. You're probably right about this, but I that doesn't mean I have to be happy about it 😆 You won me over. I'm cool with this approach.

@ofrobots ofrobots merged commit d2c325b into google:master Jun 3, 2018
@ofrobots ofrobots deleted the generate-tslint branch June 3, 2018 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants