search

pkgjs / wiby

"Will I break you" - a tool for testing dependents
Apache License 2.0
33 stars 7 forks source link

Enable linting as part of `npm test` #18

Closed dominykas closed 4 years ago

dominykas commented 4 years ago

First mentioned in https://github.com/pkgjs/wiby/pull/14#pullrequestreview-451685315:

And it will be great to add standard command run to test script

While I wouldn't call standard "great" per se :trollface:, we might as well add it to the npm test, unless there's any objections or we plan to change the style?

ljharb commented 4 years ago

.Linting should be run in pretest (or posttest, i suppose, but pretest makes more sense to me), and CI should run npm run pretest once, and should have a way to only run the tests (i usually put them in a tests-only script) on all the covered nodes.

We definitely do plan to change the style, but we can update the pretest script at that time.

rodion-arr commented 4 years ago

Can I take this task?

dominykas commented 4 years ago

pretest makes more sense to me

I don't care about it that much, but I'd rather posttest - it's more important to know early if the code does not work, rather than if the code does not look pretty enough, esp. when you're a new contributor trying to learn the codebase or just experimenting.

ljharb commented 4 years ago

The linter is about whether the code works, it's not just about how it looks. All linting rules - even style ones - prevent bugs.

dominykas commented 4 years ago

Sure, but the tests are specific to the project, while the linter rules are abstract and generic. When tests are failing - you know you broke something now. When lint is failing - you only know you might be breaking something in the future. When you're working on a piece of code, you also want to fix the things that break tests first (or, in some cases, you just want to see which tests break when you make a change) - the linter just gets in the way, esp. if it has different rules than what you're accustomed to.

You still have to fix the linter errors, but that can happen before pushing (if you intend to push at all).

ljharb commented 4 years ago

I'm not sure what you mean - the linter would only be failing because of a change you made.

In my experience, fixing linting errors often fixes the tests anyways :-) either way, locally, it doesn't really matter what order npm test runs in, since everything runs in CI, as long as everything runs locally. Most folks have the linter running in their editor anyways, long before they check the tests, so they'll see and fix the linter warnings first regardless.

dominykas commented 3 years ago

:tada: This issue has been resolved in version 0.1.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: