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 a command from a [pipx.run] entry point #602

Closed
wants to merge 2 commits into from

Conversation

uranusjr
Copy link
Member

  • I have added an entry to docs/changelog.md

Summary of changes

Test plan

Tested by running

pipx run --spec="build @ https://github.com/uranusjr/build/archive/pipx-run.zip" build --version

@uranusjr uranusjr force-pushed the run-custom-entry-points branch from 0455e99 to b466125 Compare January 20, 2021 09:45
if isinstance(command, EntryPoint):
venv.run_entry_point(command, app_args)
else:
venv.run_app(app, app_args)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
venv.run_app(app, app_args)
venv.run_app(command, app_args)

gets changed on windows. Wish single dispatch method was available pre 3.8.

Copy link
Member Author

Choose a reason for hiding this comment

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

It just occurs to me f"{app}.exe" is not actually needed. subprocess on Windows automatically resolves the extension (sine it is implemented with the CreateProcess Windows API).

Copy link
Contributor

Choose a reason for hiding this comment

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

This never returns

Why are these not annotated NoReturn instead of None, by the way? Would need fewer comments and the type checker would check the validity of the no return status. Unrelated, just thought I'd ask here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the windows test failing because of this? Or is it just tripping a unit test?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why are these not annotated NoReturn instead of None, by the way?

Probably some legacy reasons, don’t know.

Isn't the windows test failing because of this? Or is it just tripping a unit test?

The test fails because it cannot find a line in the logs; cowsay itself seems to run fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure I wrote the None return values because I was not aware there was a NoReturn. Feel free to fix that.

@uranusjr uranusjr mentioned this pull request Jan 22, 2021
1 task
@henryiii
Copy link
Contributor

henryiii commented Jan 28, 2021

What's the status of this? :)

Edit, never mind, waiting on #606, I think. I should have asked about the status of that...

@uranusjr
Copy link
Member Author

Continuing in #615.

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.

3 participants