Refactor locale validation #799
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mattias-p
force-pushed
the
validate-locale
branch
from
4067f81
to
1e4ed14
Compare
ghost
reviewed
Jun 21, 2021
ghost
mentioned this pull request
Merged
ghost
previously approved these changes
Jun 29, 2021
mattias-p
force-pushed
the
validate-locale
branch
from
7d41974
to
b6485f9
Compare
ghost
previously approved these changes
Jul 1, 2021
lib/Zonemaster/Backend/Config.pm
Outdated
| @@ -184,6 +185,11 @@ sub parse { | |||
| $obj->_set_SQLITE_database_file( $value ) | |||
| if $obj->DB_engine eq 'SQLite'; | |||
| } | |||
| if ( defined( my $value = $ini->val( 'LANGUAGE', 'locale' ) ) ) { | |||
| if ( $value eq "" ) { | |||
| push @warnings, "Use of empty LANGUAGE.locale propery is deprecated. Remove LANGUAGE.locale entry or specify LANUGAGE.locale = en_US instead."; | |||
There was a problem hiding this comment.
typos:
- propery -> property
- LANUGAGE -> LANGUAGE
matsduf
requested changes
Jul 1, 2021
lib/Zonemaster/Backend/Config.pm
Outdated
| @@ -184,6 +185,11 @@ sub parse { | |||
| $obj->_set_SQLITE_database_file( $value ) | |||
| if $obj->DB_engine eq 'SQLite'; | |||
| } | |||
| if ( defined( my $value = $ini->val( 'LANGUAGE', 'locale' ) ) ) { | |||
| if ( $value eq "" ) { | |||
| push @warnings, "Use of empty LANGUAGE.locale propery is deprecated. Remove LANGUAGE.locale entry or specify LANUGAGE.locale = en_US instead."; | |||
There was a problem hiding this comment.
The documentation says: "Leaving the locale key empty or absent is deprecated. Always configure it with supported locale tags." Having no LANGUAGE.locale is deprecated.
There was a problem hiding this comment.
I removed the deprecation of absent LANGAUGE.locale. That way we don't have to force users who don't care about native language support to add this property to their configuration files when we finally remove the feature. Having a default value of "en_US" is perfectly reasonable.
matsduf
approved these changes
Jul 2, 2021
|
I've tested this v2021.2 and it LGTM. I've run:
|
mattias-p
added
the
S-ReleaseTested
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
This PR refactors the implementation of LANGUAGE.locale.
Context
This PR extends the use of patterns for API and validation introduced in #759 and #757 to also cover the LANGUAGE.locale property.
These changes were developed in parallel with #759 and #757 as part of resolving #685. These changes are not part for solving in a technical sense, because the validations were already there. But they are in an architectural sense because they make the Config module interface more cohesive. These changes also played an important role when designing the new Config module interface, its internal structure and the new validation framework.
Changes
The methods Language_Locale_hash() and ListLanguageTags() are replaced by the LANGUAGE_locale() getter.
Call sites are updated to use the new getter instead.
The internal representation of the LANGUAGE.locale property within the Config is updated to reflect a little more of its structure as described in the configuration documentation.
The assignment of the default value for LANGUAGE.locale is placed together with the other properties.
How to test this PR
This PR is a pure refactoring so it should not affect any functionality, and it does add a few new tests at the unit level. That said it modifies code used by the
get_language_tagsandget_test_resultsRPC-API methods and we don't have any automatic tests for those.