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

Improve diagnostics from commit checker #5

Merged
merged 6 commits into from Feb 26, 2020
Merged

Conversation

nielsmh
Copy link
Contributor

@nielsmh nielsmh commented Apr 27, 2018

Instead of failing on the first problem, and then not identifying the commit that caused it, print commit ids as they are checked, and continue checking after finding problems. This should make it easier and faster to correct problems with commit messages and code formatting.

@@ -26,8 +27,11 @@ trap finish EXIT
hashes=$(git rev-list "$1")
for h in ${hashes}
do
echo Checking $h... >&2
Copy link
Member

Choose a reason for hiding this comment

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

Is there an option to only print this when there is an error?
All the check scripts stay silent when everything is ok.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe check-diff.py or check-message.py could output it themselves?

Copy link
Member

Choose a reason for hiding this comment

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

I guess easiest is to pipe all output to another temporary file, and echo that to stderr at the end, if failure was detected.

Copy link
Member

Choose a reason for hiding this comment

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

Seems marginally nasty. How about just outputting stuff on error, and checking for empty string?

Copy link
Contributor

Choose a reason for hiding this comment

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

For me always displaying the commit being checked when there's no error is ok. And it would be even better to also display the commit message and not only the hash. Easier for contributors to identify where the error is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make also prints the names of files being compiled as progress indicators. But the compiler still prefixes each message with the name of the file causing the message.

@glx22 glx22 mentioned this pull request Jan 22, 2020
@glx22
Copy link
Contributor

glx22 commented Jan 23, 2020

I'm doing some tests based on this PR on my improve branch. The ouput is visible here

@glx22 glx22 force-pushed the patch-1 branch 2 times, most recently from 9ba2747 to 9a0fe9d Compare February 23, 2020 20:45

finish() {
rm -f ${tmp_msg_file} ${tmp_diff_file}
}
trap finish EXIT

echo "::add-matcher::${HOOKS_DIR}/check-diff-matcher.json"
Copy link
Member

Choose a reason for hiding this comment

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

Do these screw with the actual githooks?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it will add a line to the output, but I don't know the effect of this line with the actual githooks (broken anyway see #11)

Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to guard them with some env variable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably, using some GH action env variable. I'll see what I can do.

nielsmh and others added 6 commits February 23, 2020 23:21
Instead of failing on the first problem, and then not identifying the commit that caused it, print commit ids as they are checked, and continue checking after finding problems. This should make it easier and faster to correct problems with commit messages and code formatting.
@glx22
Copy link
Contributor

glx22 commented Feb 23, 2020

@LordAro LordAro merged commit 9997b55 into OpenTTD:master Feb 26, 2020
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

4 participants