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

Switch to entry points and pure pyproject.toml config #474

Closed
wants to merge 2 commits into from

Conversation

segevfiner
Copy link
Contributor

@segevfiner
Copy link
Contributor Author

@kislyuk

@kislyuk
Copy link
Owner

kislyuk commented Mar 3, 2024

Sorry, I don't think we need this at this point. Is there a specific problem that you are trying to solve with this? If so, please describe the complete steps to reproduce it.


if args.dest is None:
print("Please restart your shell or source the installed file to activate it.", file=sys.stderr)
def main():
Copy link
Owner

Choose a reason for hiding this comment

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

This breaks the script by moving variables that other functions depend on from global scope into functional scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it, though there might be cleaner ways to do so.

@segevfiner
Copy link
Contributor Author

See the linked issue: pypa/pipx#1230

I'm basically unable to use this on macOS due to that.

@kislyuk
Copy link
Owner

kislyuk commented Mar 3, 2024

So you are unable to use argcomplete because of a bug in pipx where it's unable to use setuptools scripts when the absolute path to the script contains spaces?

@segevfiner
Copy link
Contributor Author

Yeah. And Homebrew Python started to force pipx.

@kislyuk
Copy link
Owner

kislyuk commented Mar 8, 2024

I plan to do some testing this weekend and see what I can do about this.

@kislyuk
Copy link
Owner

kislyuk commented Jun 12, 2024

Sorry, I don't think we will be able to merge this PR.

@kislyuk kislyuk closed this Jun 12, 2024
@segevfiner
Copy link
Contributor Author

@kislyuk Are you sure? Doing this will also allow this to work correctly in Windows...

@evanunderscore
Copy link
Collaborator

We also have other long-standing issues related to the use of plain scripts that would probably be fixed by this. I'm not up to date with pyproject.toml but a long time ago I proposed moving these to console_scripts which I assume is the same thing.

I originally wrote https://github.com/evanunderscore/argcomplete/commits/dynamic-test-shebang to address #255 but the change was never pursued as the person who reported the problem never tested it to verify it addressed their issue. If there is no specific reason to avoid making this change, it would probably be for the better and avoid more issues like this in future.

@kislyuk
Copy link
Owner

kislyuk commented Jun 30, 2024

@evanunderscore the primary reason this PR was rejected was because it tried to do too many things at the same time, causing me to lose confidence in my ability to review it and in the PR's ability to prove its correctness.

I think migrating to console_scripts is a reasonable move on its own, provided that it's done cleanly and in a staged manner that preserves the ability to review lines of scripts (e.g. first PR transforms the scripts to the format to be used by console_scripts, second PR moves the script file to its desired location and reconfigures setup.py; a separate PR may propose changes to pyproject.toml).

@kislyuk
Copy link
Owner

kislyuk commented Jun 30, 2024

@segevfiner as described in the documentation (https://github.com/kislyuk/argcomplete?tab=readme-ov-file#support-for-other-shells), windows is not directly supported by argcomplete. Only WSL is supported.

@segevfiner
Copy link
Contributor Author

@segevfiner as described in the documentation (https://github.com/kislyuk/argcomplete?tab=readme-ov-file#support-for-other-shells), windows is not directly supported by argcomplete. Only WSL is supported.

I'm not sure if any other changes then the console_scripts thing is needed though to support it.

I can split up this PR to separate PRs (Or commits, some prefer one PR with separate commits).

@kislyuk
Copy link
Owner

kislyuk commented Jun 30, 2024

A change to project policy would be needed, and that is outside the scope of this PR.

OK, please split into separate PRs including at least 3 stages as I outlined. I do not want to have one PR incorporating these changes.

segevfiner added a commit to segevfiner/argcomplete that referenced this pull request Jul 7, 2024
segevfiner added a commit to segevfiner/argcomplete that referenced this pull request Jul 31, 2024
segevfiner added a commit to segevfiner/argcomplete that referenced this pull request Jul 31, 2024
kislyuk pushed a commit that referenced this pull request Aug 2, 2024
kislyuk pushed a commit that referenced this pull request Aug 2, 2024
segevfiner added a commit to segevfiner/argcomplete that referenced this pull request Aug 2, 2024
segevfiner added a commit to segevfiner/argcomplete that referenced this pull request Aug 2, 2024
segevfiner added a commit to segevfiner/argcomplete that referenced this pull request Aug 10, 2024
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