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

Cleanly break out of windows iteration #2183

Closed
wants to merge 4 commits into from

Conversation

CristiFati
Copy link
Contributor

This is a (proposed) fix for #2163. If the user wants to stop iteration (by returning FALSE (or equivalent) from the callback) EnumWindows (&& friends) should not raise exception. It should only raise exception if callback:

Notes:

  • PyWin_SetAPIError: considering that 0 (SUCCESS) is Win's way of signaling non failure, I think that (logically) this should be the default behavior (if there's no error, don't raise any exception, especially since that message is not clear). But:

    • The function name clearly denotes an error
    • Most likely, lots of code (PyWin32 and 3rd</sup-party) depends on current behavior, I think it will trigger massive regressions

    I only added this behavior as an option (disabled by default)

  • IMPORTANT - do not merge until next question is answered:

    • As stated above, this applies to other functions from the same family (well except EnumChildWindows whose return value must be ignored). Since there are very little differences between behaviors, would it make sense to only have a function (with a couple of extra arguments) that does all the heavy lifting, and it's called by all the "visible" functions, instead of multiplicating the code in all of them?

@mhammond
Copy link
Owner

mhammond commented Feb 29, 2024

I quite like these semantics, but I'm not clear on the b/w compat story. Although you added this as an "option", each call site is what determines this option.

In other words, I'd like to understand how your tests change if you do not ever specify that new bool.

@CristiFati
Copy link
Contributor Author

I think that design-wise the correct behavior would be to always return None when there is no error (like calling the function with the (extra) bool set to true).
But I am afraid that there is code relying on the fact that an exception is raised even if GetLastError returns 0, so I took the safe side, and I just added support for this behavior, because I think that changing it altogether might break some (external) things. As code is right now, nothing changes in all places that call PyWin_SetAPIError.
If you think that changing default behavior wouldn't be a problem I can change that bool's default value (or remove it altogether (keeping the new behavior)).

@CristiFati
Copy link
Contributor Author

Closing, as it was replaced by the 2 linked PRs.

@CristiFati CristiFati closed this Mar 4, 2024
@CristiFati CristiFati deleted the cfati_dev03 branch March 28, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants