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

Iussue #1460 #1470

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Iussue #1460 #1470

wants to merge 3 commits into from

Conversation

NerusSkyhigh
Copy link

  • I have added a news fragment under changelog.d/ (if the patch affects the end users)

Summary of changes

Changed commands/common.pyline 352 from output.append(" These apps are now globally available") to output.append(" These apps are now available") as suggested on Iussue 1460.

Test plan

Tested by running

conda create --name pipx-test python pip
conda activate pipx-test
python -m pip install -e .
python -m pip install --user nox
nox -s tests-3.12

@Gitznik
Copy link
Contributor

Gitznik commented Jul 8, 2024

Thanks for this contribution.

I personally am in favor or adding the globally in the output, if the --global flag was passed, and dropping it if it was not.

@NerusSkyhigh
Copy link
Author

Hi,
I feel like this issue is solved but is not getting accepted or rejected. Is there something I can do to improve it or speed up the reviewing project?
Thanks in advance for any indication.

@huxuan
Copy link
Member

huxuan commented Jul 10, 2024

Is there something I can do to improve it or speed up the reviewing project?

As @Gitznik mentioned, we should still keep globally when --global is passed.

@NerusSkyhigh
Copy link
Author

NerusSkyhigh commented Jul 14, 2024

Hi,
sorry for the late answer but I wasn't aware of the package argparse and it took me a while to understand how arguments are parsed.
Anyway, I don't think there is an easy way to do it. The corresponding function has signature

    python_version: str,
    python_is_standalone: bool,
    package_version: str,
    package_name: str,
    new_install: bool,
    exposed_binary_names: List[str],
    unavailable_binary_names: List[str],
    exposed_man_pages: List[str],
    unavailable_man_pages: List[str],
    injected_packages: Optional[Dict[str, PackageInfo]] = None,
    suffix: str = "",
)```
and, as far as i know, none of these contains any reference to `argparse` to check wheter the option "--global" was passed.
I think there are two possible paths:
1. Rewrite the signature of the function, with a default value and backtrace its call up to a parent function where `argparse` is available
2. Ignore all possible guidelines and just do something like

for arg in sys.argv:
if "arg" == "--global":
# print one string
else:
# print the other

but I'm not a big fan of this solution.

@Gitznik
Copy link
Contributor

Gitznik commented Jul 15, 2024

I think there are two possible paths:

  1. Rewrite the signature of the function, with a default value and backtrace its call up to a parent function where argparse is available
  2. Ignore all possible guidelines and just do something like

Definitely option 1. Somewhere up the call stack it must be known whether the install is global, so you should be able to pass that right through. There should be no need for you to interact with argparse directly.

@chrysle chrysle added the awaiting response Awaiting re-engagement by contributor label Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting response Awaiting re-engagement by contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"These apps are now globally available" is confusing now that --global is a thing
4 participants