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

feat: Refactor so that app can be packed with poetry build DO NOT MERGE #533

Closed
wants to merge 2 commits into from

Conversation

cpswan
Copy link
Member

@cpswan cpswan commented Oct 13, 2023

Rather than have fiddly install instructions it would be better to use Python's packaging tools (e.g. poetry) to package this. That Python package can then be put inside of distro specific packages (e.g. a .ipk file that can be installed with opkg on OpenWRT).

@XavierChanth @Xlin123 this shouldn't be merged until #445 is resolved, and we should then apply the pattern of changes (rather than specific commits) found here.

- What I did

Refactored into a single script.

The other option would be to create an sshnpdpy package, and then another script (in another package) that would call into it, but that's more complexity and a less good user experience.

- How I did it

Copied the SSHNPDClient class from a separate lib directory into the (renamed) sshnpdpy.py script

Added a [tool.poetry.scripts] section to pyproject.toml, which defines the entry point.

- How to verify it

@Xlin123 please try this out (on this branch) to get a feel for how things (will) work.

Run poetry build, which will create sshnpdpy-0.3.0-py3-none-any.whl in the dist directory.

That can then be installed with pip3 install dist/sshnpdpy-0.3.0-py3-none-any.whl.

And then run sshnpdpy

- Description for the changelog

feat: Refactor so that app can be packed with poetry build

@cpswan cpswan self-assigned this Oct 13, 2023
@cpswan cpswan marked this pull request as draft October 13, 2023 12:50
@Xlin123
Copy link
Member

Xlin123 commented Nov 23, 2023

@cpswan do you want me to make a commit for the merge conflicts?

@cpswan
Copy link
Member Author

cpswan commented Nov 23, 2023

@Xlin123 nope. Let's not even try to merge these changes. Please take the pattern of what's been done here and apply it to the present code. Once you have a fresh PR for the refactor I'll close this one. Or if you're not clear about what needs to be done I'm happy to start over with the latest code.

@Xlin123
Copy link
Member

Xlin123 commented Nov 23, 2023

@Xlin123 nope. Let's not even try to merge these changes. Please take the pattern of what's been done here and apply it to the present code. Once you have a fresh PR for the refactor I'll close this one. Or if you're not clear about what needs to be done I'm happy to start over with the latest code.

Ok, sounds good! I'll make a new PR for the refactor.

@cpswan
Copy link
Member Author

cpswan commented Dec 1, 2023

Overtaken by #601

@cpswan cpswan closed this Dec 1, 2023
@cpswan cpswan deleted the cpswan-refactor-script branch February 13, 2024 11:07
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.

None yet

2 participants