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

docs: Rename ap-port option as ap_port #1477

Merged
merged 2 commits into from
Jul 13, 2024
Merged

docs: Rename ap-port option as ap_port #1477

merged 2 commits into from
Jul 13, 2024

Conversation

Jiogo18
Copy link
Contributor

@Jiogo18 Jiogo18 commented Jul 3, 2024

#1420 introduced the option ac_port to specify the port used by librespot.
This option works great, except when following the documentation and example only.

In ncspot/config.toml:

  • ap-port = 443 does nothing, librespot uses port 4070
  • ap_port = 443 librespot uses HTTPS

Describe your changes

Rename ap-port option by ap_port in the documentation.

Issue ticket number and link

Checklist before requesting a review

  • Documentation was updated (i.e. due to changes in keybindings, commands, etc.)
  • Changelog was updated with relevant user-facing changes (eg. not dependency updates,
    not performance improvements, etc.)

Before merge

Should the changelog be updated too?
Changelog of 1.1.1 contains ap-port, which is misleading.

The option is named `ap-port` in librespot.

The option is named [`ap_port` in ConfigValues](https://github.com/hrkfdn/ncspot/blob/9624c03264bb4038138f03374c4bfb70db7bc850/src/config.rs#L105).

In ncspot/config.toml
`ap-port = 443` does nothing, librespot uses port 4070
`ap_port = 443` librespot uses HTTPS
@ThomasFrans
Copy link
Contributor

I'd update the changelog as well. When I added it, I based it on the keepachangelog format which states that changes to earlier parts are encouraged if they improve the changelog. It's nice if users read the changelog that it kind of serves as "brief documentation" for new features 🙂

@hrkfdn
Copy link
Owner

hrkfdn commented Jul 13, 2024

Thank you, merged :)

@hrkfdn hrkfdn merged commit 2547a4f into hrkfdn:main Jul 13, 2024
5 checks passed
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

3 participants