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

Improve configuration validation #572

Merged
merged 2 commits into from
Jan 28, 2019
Merged

Improve configuration validation #572

merged 2 commits into from
Jan 28, 2019

Conversation

onigoetz
Copy link
Contributor

Fixes #567

This pull request introduces a custom validator that replaces jest-validate

Improvements over the previous implementations are:

  • You can now set a function as a renderer
  • Validations can be more precise
  • Linters are validated (check that key is a glob, and that values are a string or array of strings)
  • valid values for renderer are now explicit, where any string was possible before
  • Check that ignore is an array of strings

I tried to use joy yup and schema-utils to express the schema to validate, but it felt more verbose and less flexible with regard to the number of possible options.

I just ensured that current tests pass, I can write more tests if needed.

@codecov
Copy link

codecov bot commented Jan 22, 2019

Codecov Report

Merging #572 into master will increase coverage by 0.12%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #572      +/-   ##
==========================================
+ Coverage   98.02%   98.14%   +0.12%     
==========================================
  Files          13       13              
  Lines         354      377      +23     
  Branches       45       52       +7     
==========================================
+ Hits          347      370      +23     
  Misses          7        7
Impacted Files Coverage Δ
src/getConfig.js 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 406a0c0...130720e. Read the comment docs.

@okonet
Copy link
Collaborator

okonet commented Jan 23, 2019

I’m wondering why you decided to ditch the jest-validate since it was giving the flexibility of providing it with updated config a and get the deprecation warnings automatically. Also lots of these now hard coded checks does make the code less maintable. Have you considered adding the fix as a special case like we did with globs?

@onigoetz
Copy link
Contributor Author

Because in the current version, jest-validate can't handle different types in the same fields, this will come in version 24 (https://github.com/facebook/jest/blob/master/packages/jest-validate/src/index.js#L18)

Also, jest-validate while being easier to setup is way less precise as it stops at the type of value in the field.

@okonet
Copy link
Collaborator

okonet commented Jan 23, 2019

I’m wondering when 24 is going to be released and if we can contribute our use case to it. The reason I pushing back is that change going to introduce a lot of chore related to the config to maintainers (reas: me) and I don’t have enough time for this right now. So if we could stick to an automated yet not perfect solution I’d prefer that instead.

@onigoetz
Copy link
Contributor Author

I'll check for alternative solutions then

@onigoetz
Copy link
Contributor Author

Hi @okonet, I changed the implementation to use yup instead.
The code is smaller in terms of boilerplate, but there is still more code than before.

In exchange, the configuration validation is more precise and I added more tests to ensure it won't take you by surprise in the future.

Copy link
Collaborator

@okonet okonet left a comment

Choose a reason for hiding this comment

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

That's much better! Thanks for working on it.

@okonet
Copy link
Collaborator

okonet commented Jan 28, 2019

BTW I noticed that Jest v24 was released but I guess jest-validate still has some limitations, right?

@okonet okonet merged commit d5e738d into lint-staged:master Jan 28, 2019
@okonet
Copy link
Collaborator

okonet commented Jan 28, 2019

🎉 This PR is included in version 8.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@onigoetz
Copy link
Contributor Author

Awesome, thanks for the release :)

I also saw it was released, It solves the issue of having multiple types for the same field, which is one part of the PR. It doesn't help have more precise diagnostics on the types and linter object

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants