Skip to content

Remove trailing space characters and add a few missing newlines #1220

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

Closed
wants to merge 1 commit into from
Closed

Conversation

aaronfranke
Copy link
Contributor

This PR does a few things related to file formatting:

  • Remove trailing space characters. These are useless and just take up disk space.

  • Ensure text files end in trailing newline characters, except for JSON files. These are important for POSIX-compliance, however, some JSON tools automatically remove them, so for JSON files we make an exception. I also didn't add them to .expected since I wasn't sure about those files. In the end the only places where newlines were missing in files where they should exist was one Python file and one Bash file.

I made these changes using a script. In case I need to rebase this PR, it is very easy to re-run the script I used. Additionally, if desired, I can add the script to the CI checks, so that these formatting checks are done automatically.

@coveralls
Copy link

coveralls commented Sep 15, 2020

Coverage Status

Coverage remained the same at 93.803% when pulling ed52338 on aaronfranke:formatting into 8954092 on open-source-parsers:master.

@dota17
Copy link
Member

dota17 commented Sep 15, 2020

Thanks, these changing looks good to me.

Actually, the Travis CI will run clang-format script, so I don't think we need another formating script now.
It seems that our clang-format script don't recognize these trailing space characters your script found, just because they follow the different rules, so I am thinking if it's possible to apply your format script to clang-format script instead of having two scripts at the same time.

@aaronfranke
Copy link
Contributor Author

aaronfranke commented Sep 15, 2020

I could add it to run-clang-format.sh if you want, but there's no way for the clang-format tool itself to do this as far as I know.

Also, I noticed that the scripts are in a folder called .travis_scripts, but it doesn't seem that this repo has Travis CI enabled. If you want I can try making the scripts run on GitHub Actions CI.

@dota17
Copy link
Member

dota17 commented Sep 15, 2020

but it doesn't seem that this repo has Travis CI enabled

what you found has happened for a couple of days(or months?), Travis CI always exists and run, (see this https://travis-ci.org/github/open-source-parsers/jsoncpp/builds/727220741), but doesn't show up in the front-end CI checklist, I haven't figured it out yet, @BillyDonahue can you take a look at this?
@aaronfranke you can find the CI run clang-format script at here.

If you want I can try making the scripts run on GitHub Actions CI.

we haven't discussed to use Actions CI, so I am not sure about this point.

@BillyDonahue
Copy link
Contributor

clang-format is just for C++. I think if we were going to enforce elimination of trailing whitespace in other kinds of files, we should name that rule something else. I'm not sure what it has to do with POSIX compliance, though.

It's probably fine as a one-shot cleanup. We'll just have to watch incoming PRs for trailing whitespace.

@aaronfranke
Copy link
Contributor Author

aaronfranke commented Sep 18, 2020

Trailing whitespace is not a matter of POSIX compliance, newlines are.

The script I used is similar to this one, but we would want to tailor it for this repo if it's desired to have the script.

@BillyDonahue
Copy link
Contributor

BillyDonahue commented Sep 18, 2020

Trailing whitespace is not a matter of POSIX compliance, newlines are.

I didn't know POSIX had an opinion. It does!

The script I used is similar to this one, but we would want to tailor it for this repo if it's desired to have the script.

We'd have to be careful about dos2unix and recode as deps. The tests will sometimes be deliberately messy or ill-formed, so we'd have to be careful not to "fix" the test input data.

@baylesj baylesj self-requested a review November 6, 2020 19:43
Also add two newlines
@aaronfranke
Copy link
Contributor Author

Any update? This should be good to go. CC @baylesj and @BillyDonahue

Travis completed successfully if you click "Details", but it's not updating here on GitHub.

cdunn2001 pushed a commit that referenced this pull request Jan 10, 2021
Also add two newlines

(rebased from `aaronfranke/formatting`)

resolves #1220
@cdunn2001 cdunn2001 closed this in be4a512 Jan 10, 2021
@cdunn2001
Copy link
Contributor

Rebased and merged. Sorry for delay.

@aaronfranke aaronfranke deleted the formatting branch January 10, 2021 04:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants