Add additional ruff suggestions #1062
Conversation
examples/python-udf-comparisons.py
Outdated
| if results is None: | ||
| results = resultant_arr | ||
| else: | ||
| results = pc.or_(results, resultant_arr) | ||
| results = ( | ||
| resultant_arr if results is None else pc.or_(results, resultant_arr) | ||
| ) |
There was a problem hiding this comment.
Can you double check this? It looks like it changed the logic slightly. If results is not None it doesn't look like this will work as before
pyproject.toml
Outdated
| #"PT001", | ||
| # "ANN204", | ||
| # "B008", | ||
| # "EM101", | ||
| "PLR0913", | ||
| "PLR1714", | ||
| "ANN201", | ||
| "C400", | ||
| # "PLR1714", | ||
| # "ANN201", | ||
| # "C400", | ||
| "TRY003", | ||
| "B904", | ||
| "UP006", | ||
| "RUF012", | ||
| "FBT003", | ||
| "C416", | ||
| "SIM102", | ||
| "PGH003", | ||
| # "B904", | ||
| # "UP006", | ||
| # "RUF012", | ||
| # "FBT003", | ||
| # "C416", | ||
| # "SIM102", | ||
| # "PGH003", | ||
| "PLR2004", | ||
| "PERF401", | ||
| # "PERF401", | ||
| "PD901", | ||
| "EM102", | ||
| # "EM102", | ||
| "ERA001", | ||
| "SIM108", | ||
| "ICN001", | ||
| # "SIM108", | ||
| # "ICN001", |
There was a problem hiding this comment.
Can you please remove the lines instead of commenting them out?
python/datafusion/context.py
Outdated
| """ | ||
| if isinstance(path, list): | ||
| path = [str(p) for p in path] | ||
| path = [str(p) for p in path] if isinstance(path, list) else str(path) |
There was a problem hiding this comment.
It looks like we're checking isinstance twice here
|
I don't understand why the ruff tests are failing. My local ruff check shows no errors. |
|
You may be using a different version of ruff. These do change from time to time as new lints get added in. |
|
The workflow is using |
|
I’m not sure then. I’m away for the weekend and won’t be able to test until Monday. I can see then if I can reproduce. |
|
The errors are due to one of the rules I enabled in one of my commits |
|
Since we know this PR only addresses a portion of the rules we're working on, it seems perfectly reasonable to put it back into the ignore list for now. |
|
Thank you for all the work on this! |
|
@timsaucer there are still several rules to be enabled. But those require significant changes. Take a look at the rule This would require us to restructure these functions. There are 15 such errors. Then there is rule Should I focus on enabling all rules anyway or should I consider the returns that we get on implementing these rules and proceed only if they are meaningful? |
2 participants
WIP
18 ruff rules enabled, 4 need consideration and others are TODO
Which issue does this PR close?
Closes #1056
Rationale for this change
Need for enabling ruff rules.
What changes are included in this PR?
Enabled suggested ruff rules and changed the code to comply with them.
Are there any user-facing changes?
None.