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

Add new channels from an add URL with the new --ch-add-url option #708

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mikeymakesit
Copy link

I added CLI option --ch-add-url with which one can provide an "add" URL to add new channel(s) to a node.

It won't make any changes to the primary channel or any existing secondary channels with the same name as a channel in the URL.

If the provided URL is not valid, it errors out with a relevant message.

@CLAassistant
Copy link

CLAassistant commented Dec 2, 2024

CLA assistant check
All committers have signed the CLA.

@ianmcorvidae
Copy link
Contributor

Giving this a quick look -- I don't like that the format of the argument is different (--ch-add-url vs. --seturl isn't obviously a pair of similar commands), but I'm not sure what would be better. I don't like --addurl very much. Maybe we could change --seturl to --ch-set-url (with --seturl as an alias, for compatibility), and add the same metavar argument there too.

I'd also like it if passing a URL in either format works with either command. Maybe we could use https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urlparse and then look at just the fragment, which is all we really need for this, if you're up for that change.

Thanks for the PR, in any case!

@mikeymakesit
Copy link
Author

mikeymakesit commented Dec 2, 2024

There's a trend with arguments for channel management being prefixed with --ch- so I followed that theme, whereas --seturl is treated in the code as a device configuration setting.

The channel sharing verbs in the iOS app are "replace" and "add" so maybe the CLI options should reflect that? For those reasons and based on your feedback, how about the following?

  1. I keep --ch-add-url and update it to detect replace-style URLs and offer a suggestion to the user, doing nothing.
  2. I create new option --ch-replace-url to do what --seturl does, but offering the suggestion to use the add-style option if an add-style URL was provided.
  3. I make --seturl an alias for the new replace option.

@ianmcorvidae looking for feedback before adding commits.

@ianmcorvidae
Copy link
Contributor

I think that's an acceptable place to start, so go for it. I don't like the usability of telling people to edit the URL if they want to change the way it works (particularly for changing a replace-style to an add-style, which is the more important direction to be able to change it) but I think it's good for the time being.

Sorry about my slow responses here, I've been slammed lately with real life. And thank you for putting in the work!

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