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

fix: github action lint #24

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: github action lint #24

wants to merge 3 commits into from

Conversation

ricochet
Copy link

Lots and lots of linting fixes.

Lots and lots of linting fixes.
@@ -0,0 +1,42 @@
repos:
Copy link
Author

Choose a reason for hiding this comment

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

This project uses [pre-commit](https://pre-commit.com/).

```bash
python3 -m pip install -U pre-commit
Copy link
Author

Choose a reason for hiding this comment

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

@pvetere FYI you'll want to run through these steps on your local machine

Copy link

@kesmit13 kesmit13 left a comment

Choose a reason for hiding this comment

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

I don't see any issues here. Looks mostly like cleanup from adding the pre-commit checks.

def abort(s):
eprint(s)
os._exit(1)
sys.exit(1)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I actually was using os._exit here intentionally to avoid having the exception bubbling out. The sys.exit call apparently causes a SystemExit exception to bubble up to the higher level handlers. In some cases, thought, I just want the program to immediately exit.

https://stackoverflow.com/questions/173278/is-there-a-way-to-prevent-a-systemexit-exception-raised-from-sys-exit-from-bei

Choose a reason for hiding this comment

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

I think we might want to look into this a bit more for a less heavy-handed way of accomplishing the same thing. os._exit() is a sledgehammer.

Copy link
Author

Choose a reason for hiding this comment

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

I'm going to punt on this for this change and revert back to os._exit().

I gotta say I'm still shocked by the number of foot guns in this language.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds like a good plan. I'll take the responsibility to go back and fix it later.

src/parse_input.py Outdated Show resolved Hide resolved
src/parse_input.py Outdated Show resolved Hide resolved
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.

3 participants