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

Server: Add --serverinfo ISO country code support #2841

Merged

Conversation

hoffie
Copy link
Member

@hoffie hoffie commented Sep 7, 2022

Short description of changes
With this change, --serverinfo gains support two-letter ISO country codes such as "de", "gb", "nl", "fr". Numeric Qt5-style codes keep working as expected.

To simplify user-side debugging, the --serverinfo parsing now contains logging for the final values (or a warning in case of parsing problems).

CHANGELOG: Server: Added support for ISO country codes (de, gb, nl, ...) in --serverinfo.

Context: Fixes an issue?
Fixes: #2362
Related: #2829 (comment)

Does this change need documentation? What needs to be documented and how?

As this is a (tiny) new feature, docs would be useful, but not mandatory. This is not a breaking change. Delaying the docs update would be no problem.

Status of this Pull Request
Ready

What is missing until this pull request can be merged?

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@hoffie hoffie added the needs documentation PRs requiring documentation changes or additions label Sep 7, 2022
@hoffie hoffie added this to the Release 3.9.1 milestone Sep 7, 2022
@hoffie hoffie added this to Triage in Tracking (old) via automation Sep 7, 2022
@hoffie hoffie marked this pull request as draft September 7, 2022 21:25
@hoffie hoffie moved this from Triage to Waiting on Team in Tracking (old) Sep 7, 2022
@hoffie
Copy link
Member Author

hoffie commented Sep 7, 2022

Marking it as Draft to prevent accidential merges before #2829 is merged. Feel free to un-mask afterwards.
As to whether this should go into 3.9.1 or not: In theory, it's a (tiny) small feature and would therefore rather belong to 3.10.0. If you (mainly @pljones and @ann0see) deem it ready to merge, feel free to include it in 3.9.1. Otherwise, please move the milestone.

@pljones
Copy link
Collaborator

pljones commented Sep 7, 2022

I'm hoping to get this in to 3.9.1 - mostly because I'd love to get away from the numbers in the server info :). 3.9.1 may be a bug fix release but I see fixing server info handling as one of the "major selling points" of the release!

@hoffie hoffie force-pushed the serverinfo-two-letter-country-codes branch from c60bc81 to a58d806 Compare September 12, 2022 20:42
@hoffie hoffie marked this pull request as ready for review September 12, 2022 20:44
@hoffie
Copy link
Member Author

hoffie commented Sep 12, 2022

Rebased and un-draft-ed as #2829 has been merged.
(I'm planning to work on #2845 in a seperate PR)

src/util.cpp Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
src/util.cpp Show resolved Hide resolved
@hoffie hoffie force-pushed the serverinfo-two-letter-country-codes branch from bf309e1 to 0a76163 Compare September 14, 2022 17:45
@ann0see ann0see self-requested a review September 14, 2022 20:14
Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Besides my comment, happy to approve.

src/serverlist.cpp Outdated Show resolved Hide resolved
Tracking (old) automation moved this from Waiting on Team to In Progress Sep 17, 2022
With this change, --serverinfo gains support for two-letter ISO country codes
such as "de", "gb", "nl", "fr".
Numeric Qt5-style codes keep working as expected.

Fixes: jamulussoftware#2362
@hoffie
Copy link
Member Author

hoffie commented Sep 18, 2022

Opened jamulussoftware/jamuluswebsite#822 for the documentation. Therefore removing the needs-documentation label here. As stated, this is totally optional and doesn't have to be integrated at 3.9.1 release time.

@hoffie hoffie removed the needs documentation PRs requiring documentation changes or additions label Sep 18, 2022
@ann0see ann0see merged commit ae8742f into jamulussoftware:master Sep 18, 2022
Tracking (old) automation moved this from In Progress to Done Sep 18, 2022
@ann0see
Copy link
Member

ann0see commented Sep 18, 2022

As it's small, I've merged it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

CLI --serverinfo: Add ISO country code support in addition to Qt Country IDs
3 participants