-
Notifications
You must be signed in to change notification settings - Fork 2
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
Cruft and ruff updates #60
Cruft and ruff updates #60
Conversation
pyproject.toml
Outdated
@@ -122,13 +121,16 @@ split-on-trailing-comma = false | |||
"D", # we don't need public-API-polished docstrings in tests. | |||
"FBT", # using a boolean as a test object is useful! | |||
"PLR", # likewise using specific numbers and strings in tests. | |||
"A", |
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.
newer versions of ruff include more rules, which were triggering on tests. This ignores them.
opts.set_capability("browserName", "Safari") | ||
|
||
IOS_CAPABILITIES = opts.to_capabilities() | ||
|
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'm more than a little surprised mypy didn't complain about this sooner. For sure selenium 4.18 would have triggered it. Perhaps it's just been that long since we updated?
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.
Perhaps! Either way, thanks for updating it.
"Target", | ||
"TargetingError", | ||
"settings", |
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.
ruff wants these in slightly different alphabetical order. it considers this "isort-style"
https://docs.astral.sh/ruff/rules/unsorted-dunder-all/
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.
Makes sense, since settings
is lower-case.
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 had a couple small questions and nitpicks, but none blocking! Happy first PR of the new year!!
rev: v0.2.0 | ||
rev: v0.9.2 |
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.
Haha, dang, i guess it's been a while. 😅
@@ -1,6 +1,6 @@ | |||
MIT License | |||
|
|||
Copyright (c) 2022-2024 Perry Goy | |||
Copyright (c) 2022-2025 Perry Goy |
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.
Happy new year! 🎉
@@ -122,13 +121,16 @@ split-on-trailing-comma = false | |||
"D", # we don't need public-API-polished docstrings in tests. | |||
"FBT", # using a boolean as a test object is useful! | |||
"PLR", # likewise using specific numbers and strings in tests. | |||
"A", # import __doc__ is shadowing python builtin |
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.
nitpick, nonblocking: I'd rather just have 2 spaces before the comment. It does look nice to line them up, but then we have to keep doing it...
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.
That's doable. It'll have to be done in the cookie cutter. I can set that for the next PR.
"Target", | ||
"TargetingError", | ||
"settings", |
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.
Makes sense, since settings
is lower-case.
opts.set_capability("browserName", "Safari") | ||
|
||
IOS_CAPABILITIES = opts.to_capabilities() | ||
|
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.
Perhaps! Either way, thanks for updating it.
@@ -71,7 +71,7 @@ def not_raises(ExpectedException: type[Exception]) -> Generator: | |||
msg = f"Incorrectly Raised {error}" | |||
raise AssertionError(msg) from error | |||
|
|||
except Exception as error: # noqa: BLE001 | |||
except Exception as error: |
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.
question: Did ruff
decide this wasn't necessary?
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.
It did! There have been a few rules over the past several months that got updated in a similar fashion.
ruff had a few updates that required changes to the cookie cutter.
This also updates the copyright date.