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

Add npm test and lint files #240

Merged
merged 4 commits into from Jun 14, 2017

Conversation

Projects
None yet
3 participants
@Elchi3
Member

Elchi3 commented Jun 12, 2017

This is largely inspired by the excellent work Roman (@lahmatiy) has done over at mdn/data#89.

@Elchi3 Elchi3 added the infra 🏗 label Jun 12, 2017

@Elchi3 Elchi3 self-assigned this Jun 12, 2017

@Elchi3 Elchi3 requested a review from escattone Jun 12, 2017

Show outdated Hide outdated test/lint.js
Show outdated Hide outdated test/lint.js
Show outdated Hide outdated test/lint.js
Show outdated Hide outdated test/lint.js
@escattone

I think I prefer using ajv-cli rather than writing our own ajv-based client (test/lint.js), but it seems that if we don't use jsonlint we have to write a client anyway. Are we using JSON.stringify(JSON.parse... rather than jsonlint because it also enforces a style of writing the JSON (e.g., two-space indentation)?

In any case, this new approach works, and returns the proper exit status on error.

Show outdated Hide outdated test/lint.js
Show outdated Hide outdated test/lint.js
Show outdated Hide outdated test/lint.js
Show outdated Hide outdated test/lint.js
Show outdated Hide outdated test/lint.js
@Elchi3

This comment has been minimized.

Show comment
Hide comment
@Elchi3

Elchi3 Jun 14, 2017

Member

Thanks for your review @escattone!

I think I prefer using ajv-cli rather than writing our own ajv-based client (test/lint.js), but it seems that if we don't use jsonlint we have to write a client anyway.

I was thinking about this, too. We want to lint browser versions as well and I think for that we would need to write more custom validation script anyway (issue #168). So this is already a good preparation for more custom linting I think.

Are we using JSON.stringify(JSON.parse... rather than jsonlint because it also enforces a style of writing the JSON (e.g., two-space indentation)?

Yes, and I think it's a nice idea.

Member

Elchi3 commented Jun 14, 2017

Thanks for your review @escattone!

I think I prefer using ajv-cli rather than writing our own ajv-based client (test/lint.js), but it seems that if we don't use jsonlint we have to write a client anyway.

I was thinking about this, too. We want to lint browser versions as well and I think for that we would need to write more custom validation script anyway (issue #168). So this is already a good preparation for more custom linting I think.

Are we using JSON.stringify(JSON.parse... rather than jsonlint because it also enforces a style of writing the JSON (e.g., two-space indentation)?

Yes, and I think it's a nice idea.

@Elchi3

This comment has been minimized.

Show comment
Hide comment
@Elchi3

Elchi3 Jun 14, 2017

Member

Rebased to make sure the latest files are OK as well. I'm happy with the output https://travis-ci.org/mdn/browser-compat-data/builds/242766986

Member

Elchi3 commented Jun 14, 2017

Rebased to make sure the latest files are OK as well. I'm happy with the output https://travis-ci.org/mdn/browser-compat-data/builds/242766986

@Elchi3 Elchi3 added this to In Progress / Review Needed in Migration of compat data from MDN pages Jun 14, 2017

@escattone

This looks great, thanks for your explanations and changes @Elchi3!

@Elchi3 Elchi3 merged commit 4a25b44 into master Jun 14, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Elchi3 Elchi3 deleted the npm-test-env branch Jun 14, 2017

@Elchi3 Elchi3 moved this from In Progress / Review Needed to Done in Migration of compat data from MDN pages Jun 22, 2017

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