-
Notifications
You must be signed in to change notification settings - Fork 39
Add Windows browser path checks to Chromium detection #1014
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
base: main
Are you sure you want to change the base?
Conversation
Enhanced _check_chromium_available to check common Windows installation paths for Chrome and Edge executables. Also added support for Playwright cache detection on Windows using LOCALAPPDATA.
|
[Automatic Post]: I have assigned @simonrosenberg as a reviewer based on git blame information. Thanks in advance for the help! |
enyst
left a comment
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.
Thank you for your contributions to Windows compatibility!
I'm not sure this the way we need to go on this... I think if branches in various tools or other code on this issue are error-prone, destined to become many, reduce visibility of other code thus maintainability.
I do understand that in the browser tool case it seems hard to avoid... but then that's always the case. Idk, I'd love to give it some thought. One small question: could we at least, if we are going to do special cases throughout the codebase, make sure to recognize them in exactly the same conditions?
IMHO though, we'll be happier in time if we don't do special cases throughout the codebase...
Removed redundant os.name check and always define Windows Chromium and Edge paths. Simplified Playwright cache candidate logic by unconditionally including the Windows path.
enyst
left a comment
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.
Thank you, I see... I guess that's just how this one goes, we have Linux paths, Mac paths, Windows paths... 🤔
I kinda liked this one too, in another PR... it's an if (unfortunately 😅), but it's in __init__.py, and it makes sure to expose exactly the necessary classes

The separation of the implementation in WindowsTerminal is great IMHO. It's the ideal, if we could keep it that way.
One reason, it seems to me, is that usually, not always but usually, it's not the same people working with one or the other. It matters, IMHO, for a developer reading the code, to be able to focus on code relevant to their work. It helps maintainability, ease of debugging, code understanding.
While I'm not fully sure yet how the end solution looks on the whole issue, IMHO separated code is better than hybrid, regarding Windows (generally speaking, not in this PR). In other places, like this, I really appreciate when these pieces are at least very clear.
Thank you, I see there's no super obvious solution here, the code asks for system-specific paths, and it's the case for Linux and Mac too.
Enhanced
_check_chromium_availableto check common Windows installation paths for Chrome and Edge executables. Also added support for Playwright cache detection on Windows using LOCALAPPDATA.