8000 Revert "pre-commit: Add checking of 'make translate' status" by jepler · Pull Request #3985 · adafruit/circuitpython · GitHub
[go: up one dir, main page]
Skip to content

Revert "pre-commit: Add checking of 'make translate' status" #3985

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 13, 2021
Merged

Revert "pre-commit: Add checking of 'make translate' status" #3985

merged 1 commit into from
Jan 13, 2021

Conversation

jepler
Copy link
@jepler jepler commented Jan 13, 2021

This reverts commit 1dda33d.

For reasons we don't understand, running check-translate in the pre-commit context doesn't work during ci, giving

OSError: Syntax error in po file (line 1)
make: *** [Makefile:251: check-translate] Error 1

(see #3984)

Instead of fixing this RIGHT NOW, just revert it. We'll do more careful testing next time.

@jepler jepler requested a review from dhalbert January 13, 2021 02:20
@jepler jepler mentioned this pull request Jan 13, 2021
Closed
dhalbert
dhalbert approved these changes Jan 13, 2021
View reviewed changes
Copy link
Collaborator
@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Thanks! Failed jobs are overflowed firmware fixed by #3983,

@dhalbert dhalbert merged commit 8f9db77 into adafruit:main Jan 13, 2021
@hugodahl
Copy link

Adding my comments here which I'd posted in #CircuitPython on Discord (here)

I have been digging into why the pre-commit hook was failing in the manner described in this PR (#3985) as well as observed in #3984, and I have a theory...

I suspect that what was happening is that the shell command that created the locale/circuitpython.pot.tmp file either didn't finish writing the file to disk, hadn't yet started, or started but the file was not flushed before the tools/check_translations.py got to the line which raised the exception. Attempting to perform the same operation (polib.pofile(...)) with a file that either does not exist at the path specified, or that is empty, results in the same exception.

HOWEVER, even if we did wait for the locale/circuitpython.pot.tmp file to be fully written to disk, the call to pre-commit would still result in an error. As part of its checks, along with running the hooks configured from the .pre-commit-hooks.yaml, is to ensure that no files were changed from the execution of the hooks (as determined through a call to git diff). If it determines that there were any changed files, the pre-commit script returns a non-zero exit code, and the pre-commit action fails. That sets us right back to where we were before.

As an alternative to that method, however, I think that perhaps there could be a separate workflow or process, which performs the operations which the pre-commit hook was intended to perform. Whatever the implementation is, it could then either add a commit to the PR, gently decline it with the corrective measures to be taken or some other option. This feels like something that could be performed by :adabot:. Maybe?

@jepler jepler deleted the revert-precommit-translate branch November 3, 2021 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhalbert dhalbert dhalbert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jepler @hugodahl @dhalbert
0