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

Fix bug in WCC module #137

Merged
merged 2 commits into from
Dec 8, 2023
Merged

Fix bug in WCC module #137

merged 2 commits into from
Dec 8, 2023

Conversation

fpreynaud
Copy link
Contributor

Fixes bug introduced by commit bfd32f1

It was causing errors like the following saying that the variable 'data' did not exist:
image

@NeffIsBack
Copy link
Contributor

NeffIsBack commented Dec 7, 2023

Thanks for the bug report!
Can you show the working example? It's really odd to me, from my understanding _data should not be defined in the scope of the return in the first place (and therefore also not data).

@fpreynaud
Copy link
Contributor Author

Sure, here's the working example:
image

What happens is (schematically)

if (condition):
    data = value # data gets defined here
else:
    for _, _, data in subkey_values(): # data gets defined here again
        # do stuff
return data

If the 'data' in the for is replaced with '_data', then when in the else statement 'data' is never defined, so the function fails to return it

@NeffIsBack
Copy link
Contributor

NeffIsBack commented Dec 7, 2023

Thanks!
Hmm that was my idea as well, but if we don't run the first if=true block data shouldn't be defined in the first place and the data inside the for loop should be thrown away. Atleast that was always my understanding so far and that's probably also why ruff complained.
Thanks for the report, I'm gonna dig deeper into that :)

@fpreynaud
Copy link
Contributor Author

If that can help you, I'm using the terminology used by Windows, where (for example) HKEY_CLASSES_ROOT\ms-msime-imjpdct is the key, EditFlags is the value, and 0x00200000 is the data:

image

But sometimes the same can be referred as HKEY_CLASSES_ROOT\ms-msime-imjpdct\EditFlags as being the key, and 0x00200000 the value, without any mention of "data".

reg_query_value accommodates for the two cases by saying: if the argument valueName is not None, we're in the first case (key, value, and data), otherwise we're in the second one (just key and value). Hence the if..else at the end.

Makes me realize I really need to comment this part of the code 😆

Copy link
Contributor

@NeffIsBack NeffIsBack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@NeffIsBack NeffIsBack added tested reviewed code Label for when a static code review was done labels Dec 8, 2023
@NeffIsBack NeffIsBack merged commit 76836dc into Pennyw0rth:main Dec 8, 2023
1 check passed
Copy link
Contributor Author

@fpreynaud fpreynaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a little problem on line 591, it should be get_value(subkey_handle)[2] instead of get_value(subkey_handle)[3], because get_value only returns 3 values, not 4.

The module is working fine for now as the if is actually never entered, but it will throw an IndexError if that ever happens.

@NeffIsBack
Copy link
Contributor

Ah yes the classic index mistake. Gonna fix it fast, thanks for pointing out

@NeffIsBack NeffIsBack mentioned this pull request Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reviewed code Label for when a static code review was done tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants