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

Code cleanups from ruff and pyupgrade #17654

Merged
merged 4 commits into from
Mar 11, 2024

Conversation

nsoranzo
Copy link
Member

  • Enable flake8-comprehensions ruff rules, but allow dict() calls that make use of keyword arguments (e.g., dict(a=1, b=2)).
  • Move out from the galaxy-util package a file that requires galaxy.files
  • Code cleanups from pyupgrade

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

nsoranzo added 4 commits March 9, 2024 19:11
but allow `dict()` calls that make use of keyword arguments (e.g.,
`dict(a=1, b=2)`).
Also fix commands for `pyupgrade` Makefile target.
Found by running:

```
make pyupgrade
```
@jmchilton
Copy link
Member

Thats for the galaxy.files fix. That looks right to me!

@jdavcs
Copy link
Member

jdavcs commented Mar 11, 2024

Would you mind waiting for #17180 to be merged first? I'm afraid this might cause conflicts and some painful rebasing.

@nsoranzo
Copy link
Member Author

Would you mind waiting for #17180 to be merged first? I'm afraid this might cause conflicts and some painful rebasing.

@jdavcs I've compared the 2 PRs side-by-side and I think the only conflict would be in lib/galaxy_test/selenium/framework.py (pretty easy to solve).

@jdavcs
Copy link
Member

jdavcs commented Mar 11, 2024

thanks for checking, @nsoranzo, then it'll be fine.

@jdavcs jdavcs merged commit 728bbeb into galaxyproject:dev Mar 11, 2024
54 checks passed
@jdavcs
Copy link
Member

jdavcs commented Mar 11, 2024

...and I merged too soon! This: return [wid for wid in sa_session.scalars(stmt)] does not translate into this: list(sa_session.scalars(stmt)): the latter will raise an error.

I'll go through this line by line and will submit a fix shortly.

@nsoranzo nsoranzo deleted the ruff_C4_pyupgrade branch March 11, 2024 20:12
@jdavcs jdavcs mentioned this pull request Mar 11, 2024
4 tasks
@nsoranzo
Copy link
Member Author

...and I merged too soon! This: return [wid for wid in sa_session.scalars(stmt)] does not translate into this: list(sa_session.scalars(stmt)): the latter will raise an error.

I'll go through this line by line and will submit a fix shortly.

Is this a SqlAlchemy 2.0 change or are these lines not tested?
The scalars results are iterable, so the syntax should be equivalent, but I am not at the computer so cannot test right now.

@jdavcs
Copy link
Member

jdavcs commented Mar 11, 2024

It's not a SQLAlchemy thing, it applies to sqlite3 as well. I've tested it - they are not directly iterable, here's one example:

-> for i, row in enumerate(results):
(Pdb) row
<sqlite3.Row object at 0x7ff558d996c0>
(Pdb) list(row)
*** Error in argument: '(row)'
(Pdb) [x for x in row]
[1, 'john', 'davis', 'sagd']

Same for SQLAlchemy's Result object. Why exactly these are not iterable when passed to list, I don't know.

@nsoranzo
Copy link
Member Author

list is a command in Pdb, as you may guess from the error. Try print(list(row))

@jdavcs
Copy link
Member

jdavcs commented Mar 11, 2024

Now, this is truly embarrassing 😆 After getting tripped by args in pdb in the past, I didn't expect to step on the same rake twice. Closing the fix PR. Thank you! :)

@nsoranzo
Copy link
Member Author

No worries, I was able to notice this because I have been confused by these Pdb weirdnesses too many times!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants