CLDR-18215 Add examples for light-second#4523
Conversation
And so on.
|
I noticed a glitch (https://unicode-org.atlassian.net/issues/CLDR-18449?filter=10033) which I believe is unconnected, where the ExampleGenerator doesn't change an example when the value changes. Not a showstopper for submission, but something we should look at. |
|
|
||
| if (amount != null) { | ||
| addFormattedUnits(examples, parts, unitPattern, shortUnitId, amount); | ||
| if (shortUnitId.equals("light-speed")) { |
There was a problem hiding this comment.
Ideally this added code should go in a subroutine. It should be possible to view an entire function on the screen without scrolling.
There was a problem hiding this comment.
I tend to agree for very long functions. But this does fit on a page.
| "inch-ofhg"); | ||
| // "liter-per-100-kilometer", | ||
| // "millimeter-ofhg", | ||
| // "inch-ofhg" |
There was a problem hiding this comment.
Is there a reason to keep the commented-out code?
There was a problem hiding this comment.
It is easier to see what changed during development, but I can remove that.
| cldrFileOverride, examples, parts, "light-speed-week", amount); | ||
| addFormattedUnitsConstructed( | ||
| cldrFileOverride, examples, parts, "light-speed-month", amount); | ||
| examples.add("Compare with:"); |
There was a problem hiding this comment.
Will this "Compare with:" (and "Used as a fallback in the following:" above) get displayed appropriately on the front end?
tools/cldr-code/src/main/java/org/unicode/cldr/util/CLDRFileOverride.java
Outdated
Show resolved
Hide resolved
srl295
left a comment
srl295
left a comment
There was a problem hiding this comment.
Merged 5 minutes before I reviewed!
There was a problem hiding this comment.
surprising. did this get deleted automatically?
| public class CLDRFileOverride extends CLDRFile { | ||
|
|
||
| /** | ||
| * Overrides the values of the sourceFile. The keys of the overrides must already be in the | ||
| * XMLSource source | ||
| */ | ||
| public CLDRFileOverride(CLDRFile sourceFile, Map<String, String> valueOverrides) { | ||
| super(new XMLSourceMapOverride(sourceFile.dataSource, valueOverrides)); | ||
| } |
There was a problem hiding this comment.
I don't see the value for this being a CLDRFile subclass, since there's only a call to the constructor.
I think better would be to just make the XMLSourceMapOverride public (and the top level class) and add (if even needed) a convenience factory function:
public static final CLDRFile forFile(CLDRFile sourceFile, Map<String, String> valueOverrides) {
return new CLDRFile(new XMLSourceMapOverride(sourceFile.dataSource, valueOverrides));
}but this isn't fundamentally a new kind of CLDRFile, so I don't think the subclass is warranted.
| overrides.keySet().stream() | ||
| .forEach( | ||
| x -> { | ||
| if (source.getValueAtDPath(x) == null) { | ||
| throw new IllegalArgumentException(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
could perhaps be a subclass, ensureNoNulls
| .forEach( | ||
| x -> { | ||
| if (source.getValueAtDPath(x) == null) { | ||
| throw new IllegalArgumentException(); |
There was a problem hiding this comment.
| throw new IllegalArgumentException(); | |
| throw new IllegalArgumentException("Null DPath "+x+" found in the source."); |
| * same, since it provides for the map. | ||
| */ | ||
| public static class XMLSourceMapOverride extends XMLSource { | ||
| private XMLSource delegate; |
There was a problem hiding this comment.
perhaps DelegateXMLSource should be modified so that this could be a subclass of it instead of largely duplicate?
| // TODO (if needed) | ||
| // call delegate.getPathsWithValue(valueToMatch, pathPrefix, result); | ||
| // remove the items that collide with the map | ||
| // then look through the map to see if any matches there need to be added |
| } | ||
| } | ||
|
|
||
| public void testLightSpeed() { |
There was a problem hiding this comment.
would be a great parameterized junit test!
4 participants
And so on.
CLDR-18215
Aside from adding to the example generate handler for unit formats:
tools/cldr-code/src/main/java/org/unicode/cldr/util/CLDRFileOverride.java
tools/cldr-code/src/main/java/org/unicode/cldr/util/UnitConverter.java
tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestExampleGenerator.java
adds a test case.
This PR completes the ticket.
ALLOW_MANY_COMMITS=true