Simplified astyle travis test to be easier to reason about #9114
Conversation
.travis.yml
Outdated
| | capture(\", (?<files>[0-9]+) files\").files" \ | ||
| || echo 0) | ||
| - >- | ||
| git diff --name-only HEAD..${TRAVIS_BRANCH} \ |
There was a problem hiding this comment.
--diff-filter=d should be kept - we should not check removed files
There was a problem hiding this comment.
Counterpoint.
If a file was removed, doesn't that mean/imply that it passed astyle to begin with and wouldn't be flagged anyways?
There was a problem hiding this comment.
True, but we got ignore list. If that one is changed (removed folder from being ignored) and files that do not comply are being removed? In general this should be fine, but still removed files should not be checked
.travis.yml
Outdated
| git diff --name-only HEAD..${TRAVIS_BRANCH} \ | ||
| | ( grep '.\(c\|cpp\|h\|hpp\)$' || true ) \ | ||
| | while read file; do astyle -n --options=.astylerc "${file}"; done | ||
| - git diff --exit-code --color |
There was a problem hiding this comment.
We do not need to report number of files needs changing? Is it sufficient to just report a failure (that was happening in the after_success).
There was a problem hiding this comment.
The downside is that this simplifies/removes the files-changed delta from the job status back into a simple pass-fail.
Should be OK . It was very useful as we had non zero value on master, now it's zero so any non ezro in PR is a failure
There was a problem hiding this comment.
...Did you just answer your own question, or am I missing something?
There was a problem hiding this comment.
Yes, rereading the description
|
👍 Let's rebase and make this to CI? |
|
@kegilbert @geky Thoughts? @SeppoTakalo I remember you asking about astyle and running it locally. Is this a good intermediate improvement? |
cmonr
force-pushed
the
astyle-simplification
branch
from
a03288e
to
dbc1a78
Compare
|
@0xc0170 Rebase complete. |
|
I restarted travis, should be green. Waiting for at least one more review and ready for CI |
cmonr
force-pushed
the
astyle-simplification
branch
from
dbc1a78
to
fd5607f
Compare
|
... I have no idea why the Travis CI tests are failing. Have ping'd Travis CI for help, but I suspect this may be something with the apt-get cache... |
|
Should now have the issue resolved. My diff resolution removed three rather important lines... |
|
CI started |
Description
Greatly simplified the commands needed to run the astyle check.
Some commands seemed to be extraneous, but that's what reviews are for :)
The downside is that this simplifies/removes the files-changed delta from the job status back into a simple pass-fail.
An example of it working can be found here: cmonr#42
In the near future, I'd like to toss together a script (Travis has a REST API), to be able to generate this kind of comment: cmonr#41 (comment) (And maaaybe auto run it? Would probably get to way too verbose)
Pull request type