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

CI should verify that files are appropriately formatted (UTF-8, newlines, whitespace...) #380

Closed
lvh opened this issue Jun 29, 2016 · 12 comments
Assignees

Comments

@lvh
Copy link
Contributor

lvh commented Jun 29, 2016

In #372, I originally suggested that maybe CI should do:

iconv -f utf-8 -t utf-8 **json > /dev/null

However, this is not the only inconsistency I tripped over. For example, at time of writing, arm-devtestlabs/2016-05-15/swagger/DTL.json contained CRLF line endings (other files use LF).

One way to do both enforce, document and automate all of this is an .editorconfig file. This lets you specify:

  • file encodings
  • line endings
  • end-of-line whitespace
  • line length (non-standard but commonly supported extension; probably not needed for this repo).

Many editors support this either natively or with a plugin. This includes the Github built in editor, Emacs, vim, Visual Studio, and Visual Studio Code.

Here's an example config:

# EditorConfig is awesome: http://EditorConfig.org

root = true

[*]
end_of_line = lf
insert_final_newline = true
trim_trailing_whitespace = true
charset = utf-8
indent_style = space

This would cover all the inconsistencies I have found in the specs so far. Using eclint, you can automatically both check and even fix all current issues; so it's easy to move to this and it's easy to fix existing issues.

@John-Hart, @lmazuel: If I write this PR, is there any chance it gets merged?

@lvh lvh changed the title CI should verify that JSON files are valid UTF-8 CI should verify that files are appropriately formatted (UTF-8, newlines, whitespace...) Jun 29, 2016
@lvh
Copy link
Contributor Author

lvh commented Jun 29, 2016

This also appears to be an issue in other PRs, e,g #379. It seems like giving people a tool to do this automagically would save a ton of developer and reviewer time :)

@kirthik
Copy link
Contributor

kirthik commented Feb 14, 2017

Moved to - Azure/autorest#1825

@fearthecowboy
Copy link
Member

Reopening -- this is a CI issue, not autorest

@salameer
Copy link
Member

salameer commented Oct 9, 2018

@bsiegel not sure if we have this already, but seems like a good investment if we don't.

@lmazuel
Copy link
Member

lmazuel commented Oct 9, 2018

I would suggest use a gitattribute files to enforce all JSON to be LF.
https://help.github.com/articles/dealing-with-line-endings/

*.json text eol=lf

@bsiegel
Copy link
Member

bsiegel commented Oct 10, 2018

We can go further, it's pretty easy to add the editorconfig file and then add a pre-commit hook that runs the files through eclint first. The only downside is that we'd need to either commit the tool to the repo or people would have to run 'npm install' locally to pull it down.

@lvh
Copy link
Contributor Author

lvh commented Oct 17, 2019

I don’t know why this was closed: the associated PR does not appear to resolve the problem that existing JSON files are in disarray, and CI does not prevent regressions.

@yungezz
Copy link
Member

yungezz commented Oct 18, 2019

HI @NullMDR could you pls have a look? before adding check, pls see how to fix existing errors. thanks.

@yungezz yungezz reopened this Oct 18, 2019
@yungezz yungezz removed the triage label Oct 18, 2019
@yungezz yungezz assigned PhoenixHe-NV and unassigned bsiegel Oct 18, 2019
@akning-ms
Copy link
Contributor

Hi @NullMDR, is this fixed?

@PhoenixHe-NV
Copy link
Contributor

We already have prettier that could do format, but for the newline and utf-8 I need to confirm if it works with current prettier.

@zhenglaizhang
Copy link
Contributor

@PhoenixHe-msft any update on this thread?

@Petermarcu
Copy link
Member

Closing as this issue is too old. If this is still an issue, we should open a new issue on the tooling.

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

No branches or pull requests