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

CLDR-15381 Java formatting with prettier, tabWidth 4, printWidth 100 #1913

Closed
wants to merge 1 commit into from

Conversation

btangmu
Copy link
Member

@btangmu btangmu commented Apr 13, 2022

-Add package.json with custom settings for .java files

-Default for printWidth is 80; setting it to 100 results in fewer line breaks

-Only java files in tools/cldr-code/src/main/java/org/unicode/cldr/util

CLDR-15381

  • This PR completes the ticket.

-Add package.json with custom settings for .java files

-Default for printWidth is 80; setting it to 100 results in fewer line breaks

-Only java files in tools/cldr-code/src/main/java/org/unicode/cldr/util
@btangmu btangmu self-assigned this Apr 13, 2022
@btangmu btangmu requested review from macchiati and srl295 and removed request for macchiati April 13, 2022 20:14
@btangmu
Copy link
Member Author

btangmu commented Apr 13, 2022

@macchiati @srl295 this is the same as #1911 except that some lines tend to be longer

@@ -216,7 +242,10 @@ public static final class AnnotationSet {
static final Factory factory = CONFIG.getCldrFactory();
static final CLDRFile ENGLISH = CONFIG.getEnglish();
static final CLDRFile ENGLISH_ANNOTATIONS = null;
static final SubdivisionNames englishSubdivisionIdToName = new SubdivisionNames("en", "main");
Copy link
Member

Choose a reason for hiding this comment

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

This is definitely splitting lines that it doesn't need to. Maybe set the width higher?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll make another PR with printWidth 120

Copy link
Member Author

Choose a reason for hiding this comment

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

See #1919 -- it leaves this line unchanged

sourceLocale.equals(XMLSource.ROOT_ID)
) {
if (
!xpath.equals(
Copy link
Member

Choose a reason for hiding this comment

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

In general, I find it harder to read, because it expands to too many lines.

Copy link
Member Author

Choose a reason for hiding this comment

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

one way to address that, in this case, might be to assign the string literal "//ldml/characterLabels/characterLabelPattern[@type="category-list"]" to a constant first

Copy link
Member Author

Choose a reason for hiding this comment

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

again, see #1919 which leaves this line unchanged

}
String flagName = flagLabel == null ? subdivisionName : initialPattern.format(flagLabel, subdivisionName);
Copy link
Member

Choose a reason for hiding this comment

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

But sometimes it is clearer.

+ "\t" + key
+ "\t" + map.get(key).getShortName()
+ "\t" + Joiner.on(" | ").join(map.get(key).getKeywords()));
System.out.println(
Copy link
Member

Choose a reason for hiding this comment

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

This is really uglier and takes too many vertical lines. Original line breaks were more principled; new ones erase that.

"👨‍⚖", "👨🏿‍⚖", "👩‍⚖", "👩🏼‍⚖",
"👮", "👮‍♂️", "👮🏼‍♂️", "👮‍♀️", "👮🏿‍♀️",
"🚴", "🚴🏿", "🚴‍♂️", "🚴🏿‍♂️", "🚴‍♀️", "🚴🏿‍♀️")) {
"💏",
Copy link
Member

Choose a reason for hiding this comment

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

This one really breaks the intent, since it flattens out the logical structure.

Copy link
Member Author

Choose a reason for hiding this comment

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

with this kind of formatter, that's bound to happen -- one approach would be to make the structure more explicit by defining separate arrays and then combining them, and that might be beneficial anyway in terms of separating the code from the data

if we really want a formatter to leave such code unchanged, it will have to be a very different kind of formatter, one that only enforces certain rules and otherwise doesn't change line-breaking or anything else. I don't know if such formatters exist...

DtdType.platform), keyboards_osx(DtdType.keyboard,
DtdType.platform), keyboards_und(DtdType.keyboard, DtdType.platform), keyboards_windows(DtdType.keyboard, DtdType.platform),
;
common_annotations(DtdType.ldml),
Copy link
Member

Choose a reason for hiding this comment

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

This is much better. The jumbled structure was from an earlier application of a formatter that arbitrarily joined lines.

Copy link
Member Author

Choose a reason for hiding this comment

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

this illustrates why we might need to compromise; the formatter has to be allowed to change line-breaking in order to fix this

maybe there are formatters that will break lines but won't join them, I don't know

// ffi
{ "ffi", "FFI", "FFi", "FfI", "Ffi", "F\uFB01", "fFI", "fFi", "ffI", "ffi", "f\uFB01", "\uFB00I", "\uFB00i", },
{
"ffi",
Copy link
Member

Choose a reason for hiding this comment

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

Another bad case. No advantage at all in splitting all these up; just wastes vertical lines.

R4<K3, K2, K1, V> item = (R4<K3, K2, K1, V>) Row.of(entry0.getKey(), entry1.getKey(), entry2.getKey(), entry2.getValue());
for (Entry<Object, Object> entry1 : (
(Map<Object, Object>) entry0.getValue()
).entrySet()) {
Copy link
Member

Choose a reason for hiding this comment

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

Why on earth is it breaking (...).method() before ")."?

Copy link
Member Author

Choose a reason for hiding this comment

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

looks like it's balancing parentheses

@btangmu btangmu closed this Sep 19, 2022
@btangmu btangmu deleted the t15381_f branch September 19, 2022 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants