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

[WIP] apply Snyk suggestions #450

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

Conversation

BishopWolf
Copy link

Clean issues caught by Snyk

@BishopWolf
Copy link
Author

BishopWolf commented Sep 5, 2024

Some issues description

  1. Use of not element in list: this must be replaced by element not in list. Explanation: the not is negating the element, you intend to negate the existence in the list, it is not the same.
  2. Use of f-strings without replacements: f-strings always calls to replacements even if there is none, this creates unnecessary overload for simple strings.
  3. Use of file open outside with statements. This is highly dangerous, it may lead to broken files.
  4. Comparing to None: None is an Object, and therefore you shall check using the is operator.
  5. Missing @staticmethods.
  6. Use of exit. This is dangerous, we must use explicitly sys.exit
  7. Use of preset words (sum, slice, list) as variable names.

@dsarrut
Copy link
Contributor

dsarrut commented Sep 5, 2024

Thanks a lot @BishopWolf ! However, we are in the process of a large refactoring #404 . It will be easier to start snyk once this PR is merged (within the next few days I think), otherwise we may have lot of conflict.

@BishopWolf
Copy link
Author

The issues are easy to fix, so you can incorporate them into the same PR. Just review the list of descriptions.

@nkrah
Copy link
Collaborator

nkrah commented Sep 5, 2024

Thanks for the suggestions. We have indeed already incorporated many of them while doing the massive refactoring, e.g. opening files within a context, etc. We'll further consider your suggestions, possibly after finishing the current refactoring.

@nkrah
Copy link
Collaborator

nkrah commented Sep 5, 2024

Concerning the exit(): We are planning to raise exceptions rather than calling sys.exit() wherever possible. What is your opinion on this?
(There are many "legacy" pieces of code that will be updated in further upcoming refactoring steps. )

nkrah added a commit that referenced this pull request Sep 5, 2024
@nkrah
Copy link
Collaborator

nkrah commented Sep 5, 2024

Just went through your suggestions and implemented (in PR #403 404) those that were not anyhow already covered by PR #403 404.

We'll have another look once the PR is done and merged.

@BishopWolf
Copy link
Author

Concerning the exit(): We are planning to raise exceptions rather than calling sys.exit() wherever possible. What is your opinion on this? (There are many "legacy" pieces of code that will be updated in further upcoming refactoring steps. )

Raising/Handling exceptions is always preferable to exit.
You must add an GateException class to be called by everything.

Rules:

  1. No exception remains unhandled
  2. No generic exceptions
  3. never leave a method silently

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