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

Check formatting early in CI #3711

Closed

Conversation

andrewhamon
Copy link
Contributor

This commit makes formatting the first check, so that CI doesn't run for
hours only to fail on style.

This commit makes formatting the first check, so that CI doesn't run for
hours only to fail on style.
@oprypin
Copy link
Member

oprypin commented Dec 15, 2016

This makes changes to the formatter impossible to test.

@andrewhamon
Copy link
Contributor Author

@BlaXpirit couldn't that be mitigated by simply running the formatter before pushing changes to the formatter?

@andrewhamon
Copy link
Contributor Author

As an added bonus, reviewers would get to see what the changes look like practice.

@oprypin
Copy link
Member

oprypin commented Dec 15, 2016

@andrewhamon, that is indeed what people do, but with this change the check will run the old formatter (before recompilation with new source) and conclude that there are differences in the result. Basically, any changes to formatter will always make this fail.

Overall I like the sentiment. I had the exact same idea, but realized that it's not so easy to do...

@andrewhamon
Copy link
Contributor Author

I see now 😅 My apologies.

So I know very little about the build process... but could things be arranged like so:

  1. make crystal build the new crystal
  2. ./bin/crystal tool format --check samples spec src check formatting
  3. make std_spec clean run std lib tests
  4. make spec doc run other specs and build docs
  5. find samples -name "*.cr" | xargs -L 1 ./bin/crystal build --no-codegen do whatever this is for

Also, why does make std_spec clean run before the new crystal is built, currently?

@oprypin
Copy link
Member

oprypin commented Dec 15, 2016

@andrewhamon there are some things about the bootstrapping process that complicate everything (add some unusual constraints). Perhaps newer standard library must always still work on older compiler, or something like that.

As I understand it:
make rebuilds crystal regardless of the following arguments, but only if there isn't already a compiled crystal.
The formatter check must always run after clean.

@andrewhamon
Copy link
Contributor Author

Hmmm... well this really isn't a huge deal or anything, but it would help when noobs like me come along and forget to format. The CI wait times are brutal.

I'm going to close this since its not super important to me and I'm about to be offline for the next few days.

@RX14
Copy link
Contributor

RX14 commented Dec 16, 2016

make doesn't always rebuild crystal, its just that the standard library has to compile with the latest release. However it doesn't have to format with the latest release.

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.

None yet

3 participants