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

Fix inconsistency in commands on/off and status. #33

Open
wasndas opened this issue Jul 7, 2022 · 3 comments
Open

Fix inconsistency in commands on/off and status. #33

wasndas opened this issue Jul 7, 2022 · 3 comments

Comments

@wasndas
Copy link
Contributor

wasndas commented Jul 7, 2022

The commands on/off are inconsistent, for on/off its:

@parser.command()
@click.argument("OUTLET", type=str)
@click.argument("PORTNUM", type=int, required=False)
@click.option(
    "--switch",
    type=str,
    help="Address this switch specifically. Otherwise the first switch "
    "with an outlet that matches NAME will be commanded.",
)

and for status its:

@parser.command()
@click.argument("SWITCHNAME", type=str, required=False)
@click.argument("PORTNUM", type=int, required=False)
@click.option(
    "-o",
    "--outlet",
    type=str,
    help="Print only the information for this outlet.",
)

eg:

lvmnps on skyw.pwi

and for status its:

lvmnps status -o skyw.pwi

@wasndas wasndas changed the title Fix inconsistency of on/off and status. Fix inconsistency in commands on/off and status. Jul 7, 2022
@albireox
Copy link
Member

I agree there's some inconsistency but I think it's for a good cause. For status I think it's more likely that one wants to get the status of all the outlets in a switch instead of an outlet. For on/off one always wants to address an outlet (I think it's very unlikely one will want to turn on/off an entire switch.

In my opinion this inconsistency is ok, but if we wanted to consolidate I would vote for keeping lvmnps on/off as it is and change lvmnps status to address an outlet by default (and change the -o flag with a -s for switch).

@wasndas
Copy link
Contributor Author

wasndas commented Jul 19, 2022

Still, its confusing.

In the original implementation from one year ago, there was no -s and -o at all.
As it is still described in the README you could use on/off/status:

  1. with the unique switch name and optional a port number.
  2. with the unique port name - which should be the preferred usage.

All switch names and all port names of all switches should be unique, lvmnps took care internally if a switch (optional with port number) or a named port was addressed.

I my opinion the normal usage with on/off/status should be via unique port names, maybe multiple port names as parameters would be even better.

If a switch (and optional with a port number) has to be addressed then an option -s should be used or just revert the interface as it was.

@albireox
Copy link
Member

Sorry for the delay replying; traveling and commissioning have kept me from LVM stuff.

I don't have a problem if you want to change the behaviour. At the end of the day most of the access will either be through other actors or via a GUI, so either solution works for me.

Not sure the current implementation is that different, though. With the exception of allowing non-unique outlet names (which I'm fine not allowing), I think the current behaviour is similar? Allowing to turn on/off multiple outlets at the same time also sounds useful.

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

No branches or pull requests

2 participants