feat(install): introduce explict post-installation step#102
feat(install): introduce explict post-installation step#102pavelfeldman merged 1 commit intomicrosoft:masterfrom
Conversation
| - name: Build package | ||
| run: python build_package.py | ||
| - name: Install | ||
| run: python -m playwright install |
There was a problem hiding this comment.
| run: python -m playwright install | |
| run: playwright install |
Since we do pip install -e . it should be available in the PATH.
There was a problem hiding this comment.
The fact that we pollute env with another executable in users's path makes me sad. I'd like to emphasize that playwright is a library, not an executable and recommend its use via -m. Once we gain a handful of cli capabilities, I'll be comfortable saying playwright in my cli!
playwright/helper.py
Outdated
| self.handler = handler | ||
|
|
||
| def raise_not_installed_error(message: str) -> None: | ||
| raise Exception(f""" |
There was a problem hiding this comment.
nit: Probably I would prefer to create a new Error class which accepts in the constructor the browser name.
There was a problem hiding this comment.
Ah might be irrelevant since it does not only accept browser names, idk.
| {message} | ||
| Please complete Playwright installation via running | ||
|
|
||
| "python -m playwright install" |
There was a problem hiding this comment.
If we compare Playwright Python with Pytest, they also use just the pytest command instead of python -m pytest. So for that I would use playwright install too.
There was a problem hiding this comment.
Same as above, let's celebrate the discoverable execution once we have cli options.
playwright/main.py
Outdated
|
|
||
| def main(): | ||
| if 'install' not in sys.argv: | ||
| print ('Run "python -m playwright install" to complete installation') |
There was a problem hiding this comment.
| print ('Run "python -m playwright install" to complete installation') | |
| print ('Run "playwright install" to complete installation') |
pavelfeldman
force-pushed
the
installation
branch
2 times, most recently
from
5c2a690 to
a3df8f0
Compare
Pull Request Test Coverage Report for Build 186220460
💛 - Coveralls |
pavelfeldman
force-pushed
the
installation
branch
from
a3df8f0 to
8272826
Compare
3 participants
Fix for #21