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

run python thru ruff check --fix --unsafe-fixes #106

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

cooljeanius
Copy link
Owner

As with cooljeanius/apple-gdb-1824#79, I'm going to have to read up on ruff's documentation to learn why it labels these particular fixes as "unsafe"...

@Elvish-Hunter
Copy link

I just randomly looked at one of the proposed changes, which is removing the t_key= and t_val= assignments from wesnoth_pretty_printers.py. This will end up in a disaster, because just three lines below there's this string interpolation:

pair = "std::pair<%s const, %s>" % (t_key, t_val)

While changing things like if x == None: with if x is None: is safe, and the same goes for True and False, everything else must be checked carefully.

@CelticMinstrel
Copy link

All the == to is changes look fine. Everything else looks downright wrong – seems like it's just randomly removing assignments. In one case it even substitutes variable = "string" with just "string" which could never possibly be right.

@ProditorMagnus
Copy link

ProditorMagnus commented Aug 18, 2024

I just randomly looked at one of the proposed changes, which is removing the t_key= and t_val= assignments from wesnoth_pretty_printers.py. This will end up in a disaster, because just three lines below there's this string interpolation:

pair = "std::pair<%s const, %s>" % (t_key, t_val)

You probably look wrong branch. Such line does not exist.

While changing things like if x == None: with if x is None: is safe, and the same goes for True and False, everything else must be checked carefully.

As mentioned in irc, == does type conversion.

@ProditorMagnus
Copy link

In one case it even substitutes variable = "string" with just "string" which could never possibly be right.

That is case of tool not being able to prove that statement has no side effects. Person would delete entire line.

@CelticMinstrel
Copy link

CelticMinstrel commented Aug 18, 2024

I don't know for sure that these changes are wrong. I only know they look suspicious, because they're deleting assignments. You'll need to check that the assigned variables are truly unreferenced after being assigned. Also, in at least some cases, if you can determine they're unreferenced, you'll want to delete the whole line instead of only the assignment.

You probably look wrong branch. Such line does not exist.

The line does exist, but in an entirely different function which does the exact same assignment just before it.

As mentioned in irc, == does type conversion.

I don't think it does type conversion, but in Python, booleans are an instance of int (probably for compatibility reasons). So the value of True really is 1, and thus they compare equal.

@cooljeanius
Copy link
Owner Author

About the removed assignments: does python have a way to mark things as intentionally unused? I'm thinking about how in C, there are casts to (void), and __attribute__((used)) and __attribute__((unused)), and #pragma unused... anything similar I could use here instead of just removing the assignments?

@CelticMinstrel
Copy link

CelticMinstrel commented Sep 19, 2024

I don't know of any such thing, no. Other than the convention of using _ as the name of a variable when you don't intend to use it, but I think that doesn't apply here.

@cooljeanius
Copy link
Owner Author

I don't know of any such thing, no. Other than the convention of using _ as the name of a variable when you don't intend to use it, but I think that doesn't apply here.

Well, I think I'll give it a try anyways to see if ruff (or any of the CI checks) will say anything about it; if it doesn't work, well, this is just a draft that I might not actually do anything with, so...

@CelticMinstrel
Copy link

Normally you'd only use _ for a loop variable, or when unpacking a tuple, but whatever…

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.

4 participants