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

Lint staged #9

Merged
merged 2 commits into from
Jun 22, 2019
Merged

Lint staged #9

merged 2 commits into from
Jun 22, 2019

Conversation

ctavan
Copy link
Collaborator

@ctavan ctavan commented Jun 17, 2019

Addresses the idea from #8 (comment)

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?

@ctavan ctavan requested review from broofa, littledan and bcoe June 17, 2019 20:35
@broofa
Copy link
Collaborator

broofa commented Jun 17, 2019

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
Copy link
Collaborator Author

ctavan commented Jun 17, 2019

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
Copy link
Collaborator

bcoe commented Jun 17, 2019

@ctavan, whoops guess I was trigger happy 😛 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
Copy link
Collaborator Author

ctavan commented Jun 19, 2019

@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
Copy link
Collaborator

broofa commented Jun 20, 2019

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

npm run build

@ctavan
Copy link
Collaborator Author

ctavan commented Jun 22, 2019

@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
Copy link
Collaborator

bcoe commented Jun 22, 2019

@ctavan 👋 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.

@bcoe bcoe merged commit 23ac500 into master Jun 22, 2019
@bcoe bcoe deleted the lint-staged branch June 22, 2019 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants