-
-
Notifications
You must be signed in to change notification settings - Fork 307
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
style: Fix if-else-block-instead-of-if-exp (SIM108) (part 3) #4570
base: main
Are you sure you want to change the base?
style: Fix if-else-block-instead-of-if-exp (SIM108) (part 3) #4570
Conversation
…/temporal/gui_support.py
…ive() in grass.temporal.AbstractDataset
On my fork I think it's bandit that will complain about the use of eval, that is only moved around |
"" if keywords is None or keywords.text is None else keywords.text | ||
) | ||
shortcut = shortcut.text if shortcut is not None else "" | ||
wxId = eval("wx." + wxId.text) if wxId is not None else wx.ID_ANY |
Check warning
Code scanning / Bandit
Use of possibly insecure function - consider using safer ast.literal_eval. Warning
idx = i - 1 | ||
else: | ||
idx = i | ||
idx = i - 1 if missingKey is True else i |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idx = i - 1 if missingKey is True else i | |
idx = i - 1 if missingKey else i |
@@ -382,10 +382,7 @@ def OnEraseBackground(self, event): | |||
|
|||
def OnSize(self, event): | |||
size = self.GetClientSize() | |||
if CheckWxVersion(version=[2, 9]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may as well just remove the check, it's too old version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without docs, I'm not sure what does the check return true for (to choose what branch). If you don't know I'll read later
@@ -294,15 +294,12 @@ def __init__(self, label=None, data=None): | |||
def label(self): | |||
return self._label | |||
|
|||
def match(self, key, value, case_sensitive=False): | |||
def match(self, key, value, case_sensitive=False) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need from __future__ import annotations
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the future is used when we want to use |
instead of Optional[]
(that needs to be imported too), plus more other syntax that was supposed to be mandatory in 3.10, but still postponed. (That future exists since 3.7).
https://docs.python.org/3/library/__future__.html#module-contents
It also uses the new behavior of considering the types as strings without evaluating for better performance, instead of evaluating the types. Type checking doesn't isn't enforced at runtime anyways, so post-poneing makes sense.
In this file, bool doesn't need extra workarounds to work correctly.
Type annotations isn't new, it was there in Python 3.5, but supported syntax evolved to be more useful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I explained in #4561, in order to have more precise validations on what simplifications could be done in regards to the types. I analyzed these and greatly helped, as it allowed type checking to understand that some variables are strings (coming from the options), and that the values from the flags are bools, not any type. So some of the conditions could assume the bool type and just use "or" instead.
Last PR of this series. It finishes by enabling checking the rule with ruff.
PRs in this series: #4562, #4561
Ruff rule: https://docs.astral.sh/ruff/rules/if-else-block-instead-of-if-exp