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.
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
Add Locale.fromSubtags and support for scriptCode. #6518
Add Locale.fromSubtags and support for scriptCode. #6518
Changes from 26 commits
e042526
4013600
813998d
fe5e110
fb72be0
c6a8b96
af40b72
488cf31
829af35
c749663
04c52e9
58bfdc6
bc4cc07
a8b2797
d6f06f5
8948502
20e8bd7
9911dfd
cb81b55
5e0def8
9f6e3e0
97ab4fe
5f625ae
3b87068
df46762
57b068e
6f07e8d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add this assert to the other constructor too while we're at it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this I then manage to bring us back to the unit test that I disliked:
expect(const Locale('en'), isNot(new Locale('en', '')));
The existing Locale constructor is being tested to support both
null
and''
, and furthermore being tested to treat these differently. Mark mentioned to me yesterday "in other languages it's been found most convenient to return empty strings for omitted subtags rather than null". I suggest Dart is quite null-happy and thus might not have the same concerns. Either way, that data point and the idea to forbid''
both speak in favour of, at the very least, not treating''
andnull
differently?So: in-scope or out of scope for this Pull Request to change existing behaviour? I'm happy to consider this via a follow-up change too...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine to remove support for
''
as a value here IMHO (i.e. fix the test to verify that the empty string throws). No valid use of this API could have done that before. Having multiple ways to express the same value seems like a likely source of bugs (e.g. they'd have different hash codes presumably, unless we went out of our way to have them have the same hash code, at which point we're just wasting cycles on something that handled essentially for free by just banning the empty string).null
meaning "absent" is idiomatic in Dart. The empty string in Dart doesn't represent absence, it represents presence with a particular value (which happens to be the empty string, but could as easily be any other string).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the assert and removed what should have been a failing unit test, though I'm not sure how to actually trigger a failure. Looks like asserts are dropped (frontend_server.dart.snapshot requires no-asserts).
From an attempt to add --enable-asserts to the dart invocation for locale_test.dart:
I'd need to look into how one might tweak the snapshot later.