Skip to content

Comments

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

Merged
ann0see merged 2 commits intojamulussoftware:masterfrom
hoffie:serverinfo-two-letter-country-codes
Sep 18, 2022
Merged

Server: Add --serverinfo ISO country code support#2841
ann0see merged 2 commits intojamulussoftware:masterfrom
hoffie:serverinfo-two-letter-country-codes

Conversation

@hoffie
Copy link
Member

@hoffie hoffie commented Sep 7, 2022 •
edited
Loading

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 marked this pull request as draft September 7, 2022 21:25
@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 •
edited
Loading

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)

pljones
pljones reviewed Sep 12, 2022
View reviewed changes
@hoffie hoffie mentioned this pull request Sep 12, 2022
Merged
6 tasks
@hoffie hoffie force-pushed the serverinfo-two-letter-country-codes branch from a58d806 to da1c9ae Compare September 14, 2022 07:38
pljones
pljones reviewed Sep 14, 2022
View reviewed changes
pljones
pljones reviewed Sep 14, 2022
View reviewed changes
@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
ann0see
ann0see approved these changes Sep 17, 2022
View reviewed changes
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.

@hoffie
Server: Add --serverinfo ISO country code support
cbe8e93 
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
Server: Log parsed --serverinfo
5b2c2de 
Related: jamulussoftware#2829 (comment)
@hoffie hoffie force-pushed the serverinfo-two-letter-country-codes branch from 0a76163 to 5b2c2de Compare September 18, 2022 17:50
@hoffie hoffie mentioned this pull request Sep 18, 2022
Closed
@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
@ann0see
Copy link
Member

ann0see commented Sep 18, 2022

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

@pljones pljones mentioned this pull request Sep 21, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pljones pljones pljones approved these changes

@ann0see ann0see ann0see approved these changes

Assignees

@hoffie hoffie

Labels

None yet

Projects

None yet

Milestone

Release 3.9.1

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

@hoffie @pljones @ann0see