brianhealey / pyamplipi

Python API for interacting with the AmpliPi Multizone Audio Controller
MIT License
3 stars 4 forks source link

Implementing a complete CLI support for the web-api #17

Closed marc-portier closed 1 year ago

marc-portier commented 1 year ago

fixes a bunch of outstanding things:

linknum23 commented 1 year ago

Let me know when this is ready and I'll review it!

marc-portier commented 1 year ago

Please do review and consider the merge already

Which makes me think about next steps on this library: after this rather big merge (which turns out more important because of the 0.1.9 update than because of the cli support IMHO) I would like is us to (1) discuss the testing (#12) approach and (2) decide on some kind of general / long-term release plan (i.e. how would we want to have the lib follow/sync up with the amplipi versions) (3) list which of the open minor stuff we want to include in the short-term next release?

BTW: the above does mean I want to be involved and active in collaborating on those goals. But I also understand if my current planning, eagerness and availability doesn't fit your schedule - conversation to be had should make details like that clear?

Finally, my personal plan is now to start applying all of this stuff into my personal setup / local home development first. I assume that work might show other bugs / feature requests from the real-life testing it will involve. Also: I was planning to share my design goals and plans on the community discord one of these days - that might lead to consider "higher level" functionality to be welcome as part of this lib rather than in my personal repo only?

linknum23 commented 1 year ago

I'm just starting to play with the cli after making some changes to make it work in 3.8. This is fantastic! The output looks great. What python version are you developing in? I noticed the setup.py still has 3.5/3.6 listed but I'm assuming this is being developed in 3.9-3.11?

marc-portier commented 1 year ago

well, glad you like it (the nice tables come - almost for free - from the tabulate package.)

I am on ubuntu 22.04 which comes with python 3.10.6 am eager to learn how I can easily dev/test for multiple py versions (maybe also something the CI approach can help with?)

linknum23 commented 1 year ago

On stack overflow I saw some mention of a way to hook into argparses on_exit and on_error handling. I'll see if I can find it again.

As for the type checker I'll add it. It's just mypy with some settings I think? On Tue, Jan 3, 2023, 18:13 Marc Portier <> wrote:

@.**** commented on this pull request.

In pyamplipi/docs/cli.md https://github.com/brianhealey/pyamplipi/pull/17#discussion_r1061026229:

@@ -0,0 +1,184 @@

+# Commane Line Interface

We've been playing around with the interface for a couple of hours and everything is very usable.

I found a couple of small issues in shell mode that kicked me out:

Entering the topic correctly but forgetting the action:

yes, and this moves the argparse to go into --help mode which in turn does a system exit I tried resolving that through exit_on_error = False but only helped for parse-errors that don´t trigger the print_help()

haven´t found another solution yet

Also when running this through my standard typechecker setup of pylint + mypy I found and fixed some type issues I added them to the PR in the hopes that they are helpful. Feel free to remove them.

interesting.

if that typechecking is available as a standalone python package than we should look into (1) adding it to the requirements-dev.txt and (2) include in the make check

I have no intention at all to remove your additions / modifications -- will investigate later, but I am all ok with you proceeding with the merge...

— Reply to this email directly, view it on GitHub https://github.com/brianhealey/pyamplipi/pull/17#discussion_r1061026229, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEZPOYH7JOGP7XW3MDR5SDWQSXBLANCNFSM6AAAAAATNBKA2E . You are receiving this because you commented.Message ID: @.***>

linknum23 commented 1 year ago

I added the mypy checks, and fixed some of the problems it identified. I'll merge this in so we can keep the momentum on this!