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/style fixes #719

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

Fix/style fixes #719

wants to merge 6 commits into from

Conversation

kwikwag
Copy link
Contributor

@kwikwag kwikwag commented Oct 25, 2024

1. Content and background

Some coding style suggestions (including one possible bugfix).

2. Summary of corrections

I went over the following:

  1. Removed Optional where it was incorrectly specified in a type annotation Added some Literal type annotations.
    Optional should only be specified when the argument can receive None as a value, which was not true in most cases. From Python docs:

    Optional[X] is equivalent to X | None (or Union[X, None]).

  2. Moved traceback printing calls to logging.

  3. Raise exceptions instead of sys.exit(). This makes it easier to use within another script.

  4. Do not use base except. This has the side-effect of catching KeyboardInterrupt too which is most likely not what is wanted.

  5. Ran black for styling and went over some of flake8 linting comments (e.g. remove f prefix when the f-string has no placeholder, etc.).

The latter revealed that a call to gs.export_onnx() was using an undefined variable graph. This bug was introduced in commit 85147eb,

I had a mistake in one of the commits with a call to warn_tb that I didn't correct until a later commit. But the final results is OK.

If need be, I can prepare a pull request with only a subset of these changes. I avoided making any functional changes, only focusing on style.

@PINTO0309
Copy link
Owner

PINTO0309 commented Oct 25, 2024

I don't have any objections to the proposed changes themselves, but there are too many changes to review. Can you split up the pull requests into separate requests for each requirement? The scope of the impact is too large.

Repository owner deleted a comment from github-actions bot Oct 30, 2024
Repository owner deleted a comment from github-actions bot Nov 5, 2024
@PINTO0309 PINTO0309 added the TODO TODO label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO TODO
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants