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

Added schema variable and added more logging #3449

Merged
merged 4 commits into from
Dec 9, 2019
Merged

Added schema variable and added more logging #3449

merged 4 commits into from
Dec 9, 2019

Conversation

jef
Copy link
Member

@jef jef commented Dec 6, 2019

Description

The plugin does a great job updating, but if I wanted to use the 443 port (https) it fails. Example, going to https://google:80 fails because it doesn't know how to forward this. So in the users' case, they would have to assume that giving port 80 in the config will forward to 443 using https properly. This just makes it more obvious.

I also added some more logs to make it more obvious to the user what's going on if they use the --verbose option. And if they don't, then it will send a log.info for success cases.

In terms of logging, I think I did this right. Please let me know if I didn't. Also first time contributing something in Python -- sorry if it doesn't look right. Let me know if I need to update anything!

@sampsyo
Copy link
Member

sampsyo commented Dec 6, 2019

Thanks for pointing this out; this could obviously be better!

I have a slightly different proposal to run by you: instead of the separate and varied config options host, port, and contextpath, and then layering on URL schemes on top, we should consider deprecating all of them and replacing them with a single url config parameter. That is, you'd just use https://example.com:123 if that's what you need. In addition to being simpler, this would have the added benefit of letting you use SSL even on non-default ports.

@jef
Copy link
Member Author

jef commented Dec 6, 2019

we should consider deprecating all of them and replacing them with a single url config parameter.

Good idea. In that case, I'll make some changes and have you take a look again!

@jef
Copy link
Member Author

jef commented Dec 6, 2019

Let me know what you think -- maybe needs other changes. Definitely need a documentation update.

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Looking good; thanks for exploring this! I have a few suggestions within.

beetsplug/subsonicupdate.py Outdated Show resolved Hide resolved
beetsplug/subsonicupdate.py Outdated Show resolved Hide resolved
beetsplug/subsonicupdate.py Outdated Show resolved Hide resolved
beetsplug/subsonicupdate.py Outdated Show resolved Hide resolved
beetsplug/subsonicupdate.py Outdated Show resolved Hide resolved
beetsplug/subsonicupdate.py Outdated Show resolved Hide resolved
beetsplug/subsonicupdate.py Outdated Show resolved Hide resolved
beetsplug/subsonicupdate.py Outdated Show resolved Hide resolved
@jef
Copy link
Member Author

jef commented Dec 6, 2019

Oh wow... Sorry, I went quite a bit ahead before you commented.

@jef
Copy link
Member Author

jef commented Dec 6, 2019

Okay! I'm done playing with it now if you want to take another review. Thanks!

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Nice work! Here are just a few more organizational suggestions.

beetsplug/subsonicupdate.py Outdated Show resolved Hide resolved
beetsplug/subsonicupdate.py Outdated Show resolved Hide resolved
beetsplug/subsonicupdate.py Outdated Show resolved Hide resolved
beetsplug/subsonicupdate.py Show resolved Hide resolved
docs/plugins/subsonicupdate.rst Outdated Show resolved Hide resolved
docs/plugins/subsonicupdate.rst Outdated Show resolved Hide resolved
docs/plugins/subsonicupdate.rst Outdated Show resolved Hide resolved
docs/plugins/subsonicupdate.rst Outdated Show resolved Hide resolved
Jef LeCompte added 2 commits December 6, 2019 18:30
Also updated documentation to note the password options.
@jef jef requested a review from sampsyo December 6, 2019 23:38
user: username
pass: password
contextpath: /subsonic

\* NOTE: The pass config option can either be clear text or hex-encoded with a "enc:" prefix.
Copy link
Member Author

@jef jef Dec 6, 2019

Choose a reason for hiding this comment

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

From their API docs.

Copy link
Member

Choose a reason for hiding this comment

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

Cool! Buy maybe this sentence should go below, in the bullet for the pass configuration option, rather than by itself here.

@jef
Copy link
Member Author

jef commented Dec 7, 2019

Not sure if I understand the CI problems... Is it trying to download packages that it can't? I don't think my PR is related.

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Awesome; this looks great!! Here are just a couple of very low-level suggestions. Also, would you mind adding a quick changelog entry to docs/changelog.rst?

docs/plugins/subsonicupdate.rst Show resolved Hide resolved
user: username
pass: password
contextpath: /subsonic

\* NOTE: The pass config option can either be clear text or hex-encoded with a "enc:" prefix.
Copy link
Member

Choose a reason for hiding this comment

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

Cool! Buy maybe this sentence should go below, in the bullet for the pass configuration option, rather than by itself here.

beetsplug/subsonicupdate.py Outdated Show resolved Hide resolved
beetsplug/subsonicupdate.py Outdated Show resolved Hide resolved
beetsplug/subsonicupdate.py Outdated Show resolved Hide resolved
@jef jef requested a review from sampsyo December 9, 2019 12:13
sampsyo added a commit that referenced this pull request Dec 9, 2019
Added schema variable and added more logging
sampsyo added a commit that referenced this pull request Dec 9, 2019
sampsyo added a commit that referenced this pull request Dec 9, 2019
@sampsyo sampsyo merged commit 89f21d9 into beetbox:master Dec 9, 2019
@sampsyo
Copy link
Member

sampsyo commented Dec 9, 2019

Awesome; thank you again for seeing this through to its conclusion! ✨ ✨ Merged with a changelog entry.

@jef
Copy link
Member Author

jef commented Dec 9, 2019

Thanks for the support! It was fun doing Python for the first time.

@jef jef deleted the patch-1 branch December 9, 2019 21:00
ybnd pushed a commit to ybnd/beets that referenced this pull request Jan 20, 2020
ybnd pushed a commit to ybnd/beets that referenced this pull request Jan 20, 2020
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