tc39 / proposal-uuid

UUID proposal for ECMAScript (Stage 1)
463 stars 7 forks source link

Lint staged #9

Closed ctavan closed 5 years ago

ctavan commented 5 years ago

Addresses the idea from https://github.com/bcoe/proposal-standard-module-uuid/pull/8#discussion_r294347624

Not sure though how I feel about adding this amount of tooling to something that is mostly just a README 🤷‍♂

$ du -sh node_modules
 74M    node_modules

WDYT?

broofa commented 5 years ago

heh You beat me to it.

My PR is a little lighter weight, mostly by virtue of enforcing lint at in the build step rather than on commit. I don't have much experience with either prettier or markdownlint, so no strong opinions on which is better. 'Not gonna argue strongly for one over the other here. :-)

ctavan commented 5 years ago

Huh 😛, in German there's a saying that goes "Zwei Dumme, ein Gedanke", which literally translates to "two fools, one thought" but it's meant in a positive way (Google just told me the correct translation is "great minds think alike" which I feel less comfortable with because I'd rather describe myself as a fool than a great mind…)

I also don't have a strong opinion on this topic and neither do I have a lot of experience with these tools. I'll happily leave that up for @bcoe to decide 🙇.

bcoe commented 5 years ago

@ctavan, whoops guess I was trigger happy :stuck_out_tongue: landed @broofa's work.

Mind rebasing? looks like the main difference is that you actually applied the linter and fixed the doc on master, also you'd like it to be a width of 99 not 100?

ctavan commented 5 years ago

@bcoe I did rebase but don't you think this is kinda redundant now? @broofa's approach and mine were different in two dimensions:

@broofa:

  1. Lint (no automatic formatting)
  2. Pre-test script (where/by whom is that run as we currently don't have a CI pipeline?)

@ctavan:

  1. Prettier = automatic formatting
  2. lint-staged, i.e. applied to all staged files before each commit

I think we should, for each dimension, decide for one approach:

  1. Do we want linting or automatic formatting?
  2. Do we want it to be tested passively (e.g. in a CI pipeline) or enforced actively before each commit?

The difference of 99 vs. 100 is an artifact of how vim handles line wrapping when you set textwidth=100, it will ensure that no line exceeds 99 characters, as the 100th character will always cause a wrap. I can change that of course if you prefer to have up to 100 characters 😉

broofa commented 5 years ago

Pre-test script (where/by whom is that run as we currently don't have a CI pipeline?)

npm run build

ctavan commented 5 years ago

@broofa my concern was not so much how to manually run this script, my concern was rather that we currently have no automation in place that will ensure that the stuff we merge into master adheres to our conventions… Which is why I went for the lint-staged approach in the first place. It's of course a weaker guarantee than a true CI step linked into Github PRs but I think it's better than relying on somebody manually running a command.

bcoe commented 5 years ago

@ctavan :wave: I just added TravisCI:

https://travis-ci.org/bcoe/proposal-standard-module-uuid

I think my preference would be that we don't automatically fix the formatting on commit, but that we have a npm run fix command that allows folks to fix formatting with one API call.

I could be swayed in either direction though.