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

Allow to run the module without installing it #138

Merged
merged 2 commits into from
Jan 28, 2024
Merged

Conversation

n-peugnet
Copy link
Contributor

@n-peugnet n-peugnet commented Dec 3, 2023

As explained previously, I want to add the click generated bash completion script to the Debian package. Here is the change I sent to the Debian maintainer of synadm : https://salsa.debian.org/matrix-team/synadm/-/merge_requests/1

The only way I found to make it work was to add a "main" file that allows to run synadm without installing it. As I thought it could be useful outside the Debian package, here is the forwarded patch, that allows to run synadm directly with python -m synadm.

I am no expert in Python so this might not be the ideal way to do it.

@JOJ0 JOJ0 self-requested a review December 3, 2023 23:34
Copy link
Owner

@JOJ0 JOJ0 left a comment

Choose a reason for hiding this comment

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

Hmm interesting, not possible for me to try out myself atm but wouldn't that work already:

python -m synadm.cli

@JOJ0
Copy link
Owner

JOJ0 commented Dec 3, 2023

but that would require it to be installed already...

the debian package requires it to be able to run without installing? why? during packaging?

@n-peugnet
Copy link
Contributor Author

the debian package requires it to be able to run without installing? why? during packaging?

Not the Debian package directly, but the way Click based CLI's shell completion work is by calling the program with a specific environment variable set to generate the completion script.

I make this call in Line 4 of debian/rules, and I didn't find any better way to make it than by making the package executable with python -m. I also tried pybuild's AFTER_INSTALL hook but didn't manage to make it work without this change either.

But if you don't want this change, it can also stay a Debian only patch, as it is very small and should not change too often. And if you have better suggestions I am also interested.

@JOJ0
Copy link
Owner

JOJ0 commented Dec 20, 2023

Hmm interesting, not possible for me to try out myself atm but wouldn't that work already:

python -m synadm.cli

Nope that doesn't work :-)

$ python -m synadm.cli -h
/home/jojo/.venvs/synadm310/bin/python: No module named synadm.cli.__main__; 'synadm.cli' is a package and cannot be directly executed

@JOJ0
Copy link
Owner

JOJ0 commented Dec 20, 2023

the debian package requires it to be able to run without installing? why? during packaging?

Not the Debian package directly, but the way Click based CLI's shell completion work is by calling the program with a specific environment variable set to generate the completion script.

I make this call in Line 4 of debian/rules, and I didn't find any better way to make it than by making the package executable with python -m. I also tried pybuild's AFTER_INSTALL hook but didn't manage to make it work without this change either.

But if you don't want this change, it can also stay a Debian only patch, as it is very small and should not change too often. And if you have better suggestions I am also interested.

Hi @n-peugnet, I'm not opposed to this change at all. Please let's add it. Some things I'd like to add within this PR though:

Some docstrings in the new module __main__ describing why it is here. You could even state the initial reasons: Running without installing, Debian packaging

Also I would like to add to the docs, the new possibility to run synadm programatically, it might be of interest to other packagers or to people who'd like to integrate synadm into their own programs. Not sure where in the docs I would see that located though...

And also, but this may be off-topic here, but I would like to include it in the PR while we are at it and we have the expert at hand already (you ;-), let's add to the docs what commands and setup would be required to make synadm bash completion work when synadm is installed via the pip package. Maybe have the concrete commands for bash and zsh right in our docs but as well linking to the Click docs: https://click.palletsprojects.com/en/8.1.x/shell-completion/

If you find time to help us with these things within this PR that would be wonderful. Certainly not urgent since we are not fixing a bug here but at some point this or next year?

@JOJ0
Copy link
Owner

JOJ0 commented Dec 20, 2023

I realized where in the docs the main module could be included. Since it is very much only interesting for developers, it would fit as a new page in the "package documentation" section. Creating a file named doc/source/synadm.module.main.rst and including this should do the trick:

Synadm Main Package
==================

.. automodule:: synadm.__main__
   :members: root
   :undoc-members:
   :show-inheritance:
   :private-members:
   :member-order: bysource

The docstring you add to the source would then automatically get rendered within our docs.

Finally adding a line

synadm.module.main to the index file below here:

synadm.module.cli
synadm.module

and it should be accessible in the TOC.

You can build the docs locally (but I see that a howto for that is missing in our docs, I should add that) or tell me to check out your branch and I'll test it.

@JOJ0
Copy link
Owner

JOJ0 commented Dec 20, 2023

update again: @n-peugnet I'm not 100% sure about my previous post's description #138 (comment) anymore. Maybe forget about it for now and just add the docstring in __main.py__ and a description about how to set up bash/zsh completion. The latter you could even just add as a comment here, and I'll handle the adding to docs later in master or a new PR myself.

@JOJ0
Copy link
Owner

JOJ0 commented Jan 24, 2024

Hi @n-peugnet, I just wanted to the following commit to this PR to finalize it: 4e41cd5

It seems I'm not allowed to push that commit to your branch. Would it be possible for you to allow "maintainer changes" on your end? https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests

What I did is adding you as the author of the file and I elaborated on why this file is here. Please review and tell me if you would agree with these changes.

Thanks!

@n-peugnet
Copy link
Contributor Author

Hi @n-peugnet, I just wanted to the following commit to this PR to finalize it: 4e41cd5

[...]

What I did is adding you as the author of the file and I elaborated on why this file is here. Please review and tell me if you would agree with these changes.

This is perfect ! Sorry, I didn't answer you previous message, thank you for doing this yourself.

It seems I'm not allowed to push that commit to your branch. Would it be possible for you to allow "maintainer changes" on your end? https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests

Well, this is strange, I don't have this checkbox on this pull request page. It might be because I forked the repo in an organisation. I think I would need to create a new pull request and close this one. But if you are OK to simply merge 4e41cd5 into main, this is fine by me.

@n-peugnet
Copy link
Contributor Author

I figured I could simply pull your commit in my branch.

@JOJ0
Copy link
Owner

JOJ0 commented Jan 28, 2024

Yeah a cherry-pick would do it. Thanks!

@n-peugnet
Copy link
Contributor Author

Yeah a cherry-pick would do it. Thanks!

Already done, in case you didn't see

@JOJ0
Copy link
Owner

JOJ0 commented Jan 28, 2024

Ah yeah missed it. Many thanks!

@JOJ0 JOJ0 merged commit e556263 into JOJ0:master Jan 28, 2024
2 checks passed
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.

2 participants