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

Update or add a JSON linting step in Travis CI to check for formatting issues #1324

Closed
davidklebanoff opened this issue Mar 25, 2019 · 5 comments
Labels
meta Issues or pull requests regarding the project or repository itself

Comments

@davidklebanoff
Copy link
Contributor

Update the JSON lint schema to check for formatting issues. This can include extraneous whitespace, improper indentation, etc.

@davidklebanoff davidklebanoff added the meta Issues or pull requests regarding the project or repository itself label Mar 25, 2019
@davidklebanoff davidklebanoff mentioned this issue Mar 25, 2019
3 tasks
@davidklebanoff
Copy link
Contributor Author

After a little reading, it would appear that JSON schema validation doesn’t have the ability to check formatting, and the linter we use jsonlint currently does not support formatting checks either. So this task may have a little more depth than it initially appears (not a quick toggle or schema update).

Additionally it's worth noting that there are two versions of jsonlint, there’s the original jsonlint
which appears to be abandoned and there’s jsonlint2 (which we are currently using) which is simply a fork that updates the nomnom dependency.

#527, #528, #529 are where json linting first came into the project.

@davidklebanoff davidklebanoff changed the title Update the JSON lint schema to check for formatting issues Update or add a JSON linting step in Travis CI to check for formatting issues Mar 25, 2019
@ericcornelissen
Copy link
Contributor

I've looked around a bit but there seems to be no available linter that just lints white space no matter the filetype/syntax. I've tried some things with eslint-plugin-json and jshint-json but to no avail.

This tool, grunt-wslint comes closest to what I think we want. Unfortunately it is a Grunt plugin 😞 I personally feel a bit weird adding a Gruntfile.js just for this plugin...

Also, I want to add to this that we might consider adding some kind of linting that checks if the icon SVGs do not have a trailing newline.

@ericcornelissen
Copy link
Contributor

ericcornelissen commented Apr 5, 2019

Update, it seems it is possible to achieve what we want using editorconfig-checker in which case we have to add an .editorconfig file (read more about EditorConfig here). Personally I prefer that over adding Grunt as a devDependency and adding a Gruntfile.js, and it is more convenient than building a linter from scratch, but that is of course debatable.

I'm currently considering the following EditorConfig configuration. This should correctly lint the _date/simple-icons.json file for trailing whitespace (and some other things). However, it does not seem to want to lint SVG files (for the trailing newline issue), but I contacted the maintainers about that.

root=true

[*]
charset=utf-8
insert_final_newline=true

[LICENSE.md]
indent_size=unset
indent_style=space

[*.{json,yml}]
indent_size=2
indent_style=space

[.github/**/*.md]
trim_trailing_whitespace=false # Templates with trailing whitespace are more usable

[_data/simple-icons.json]
insert_final_newline=false
indent_style=space
indent_size=4
trim_trailing_whitespace=true

@awwsmm
Copy link
Contributor

awwsmm commented Sep 28, 2019

I'm trying to submit a PR with a new icon but Travis CI keeps rejecting it for having a newline at the end of the SVG. But I've deleted the newline and I'm still getting that error. Can this barrier be removed? At most this adds an additional byte or two to the size of the SVG file.

There also seem to be build errors related to code which I've not touched:

SyntaxError: Unterminated string constant
    at JS_Parse_Error.get (eval at <anonymous> (/home/travis/build/simple-icons/simple-icons/node_modules/uglify-js/tools/node.js:20:1), <anonymous>:71:23)
    at formatError (util.js:609:16)
    at formatValue (util.js:513:18)
    at inspect (util.js:293:10)
    at format (util.js:160:12)
    at Console.warn (console.js:145:21)
    at data.icons.forEach.icon (/home/travis/build/simple-icons/simple-icons/scripts/build-package.js:57:15)
    at Array.forEach (<anonymous>)
    at Object.<anonymous> (/home/travis/build/simple-icons/simple-icons/scripts/build-package.js:48:12)
    at Module._compile (module.js:653:30)
  message: 'Unterminated string constant',
  filename: '0',
  line: 4,
  col: 7,
  pos: 76 }

@ericcornelissen
Copy link
Contributor

I'm trying to submit a PR with a new icon but Travis CI keeps rejecting it for having a newline at the end of the SVG. But I've deleted the newline and I'm still getting that error. Can this barrier be removed? At most this adds an additional byte or two to the size of the SVG file.

This seems to be a common problem (mostly caused by editors - including GitHub's IDE - always adding a final newline by default, though installing EditorConfig for your editor would immediately solve that problem). Given the trouble it is causing contributors I'm fine with getting rid of the requirement 👍

The build error is peculiar, let's continue that discussion on the Pull Request itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues or pull requests regarding the project or repository itself
Projects
None yet
Development

No branches or pull requests

3 participants