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 formatting script and use GitHub Actions #200

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aaronfranke
Copy link
Contributor

@aaronfranke aaronfranke commented Feb 19, 2021

This is a follow-up to #115 and #136. Over time I've made improvements to this script, so this PR updates it in this repo.

The tail command has been replaced with perl, which is more portable, as it also runs on macOS. There is also a set pipefail and IFS definition that properly define those for any OS that doesn't already do so. I also added a .gitattributes file, which tells Git to resolve line ending encoding problems automatically.

Aside from the script, the CI has been updated to use GitHub Actions. This is nicer than Travis or AppVeyor, the status and details are nicely integrated into GitHub, and if there are multiple jobs they can all be displayed individually. The CI won't run on this PR because it only runs if the config is already in the repo, but it will run after this PR is merged.

The second commit in this PR is just running the script, which removes many useless trailing spaces and tabs.

@UebelAndre
Copy link
Contributor

Should this be rebased to fix the new formatting errors?

@aaronfranke
Copy link
Contributor Author

@UebelAndre Thanks for the ping, I updated the PR. If it needs updating in the future, let me know.

@UebelAndre
Copy link
Contributor

I think it's up to @richgel999 but I think it'd be good to separate linting from the build

@UebelAndre
Copy link
Contributor

Does this require any manual action to enable? Or are GitHub actions enabled automatically based on something in the repo?

@aaronfranke
Copy link
Contributor Author

@UebelAndre It should be automatically enabled, but there are also repo settings to turn it on and off.

@UebelAndre
Copy link
Contributor

Just wondering if there's any info @richgel999 should have when should this PR get merged to make sure it's properly enabled

@UebelAndre
Copy link
Contributor

@richgel999 hey, do you have a sec to look at this? It'd be really nice to separate the linting from the Linux build so it's more obvious what's wrong and when

@richgel999
Copy link
Contributor

Yes, I'm working down the PR list. Please Standby.

@richgel999
Copy link
Contributor

I will run the formatting script on v1.16 - the January release.

@richgel999 richgel999 added the enhancement New feature or request label Dec 19, 2021
@aaronfranke
Copy link
Contributor Author

@richgel999 @UebelAndre I know it's been a long time, but I updated the PR to use Python and pre-commit instead of a shell script. Compared to the old version of this PR, it gives the following advantages:

  • Support for running the formatting script on Windows.
  • Support for running the formatting script on MacOS/Linux without installing any OS-level dependencies: Simply install pre-commit via pip install pre-commit. The only OS-level dependency you need is to have Python installed. Then pre-commit run --all-files in the repo to run.
  • Support for automatically running the formatting script on changed files when making a commit using Git's pre-commit hooks feature, if you opt-in via pre-commit install in the repo.

Copy link
Contributor

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants