From 9a02ba9c8dfb74d3f1036cd8140546e78c811dfa Mon Sep 17 00:00:00 2001 From: Albert Wolszon Date: Thu, 16 Mar 2023 16:34:06 +0100 Subject: [PATCH 1/3] Update README placeholders section and syntax diagrams --- README.md | 14 +- art/count-placeholder-syntax.svg | 297 ++++++++++++--------------- art/placeholder-syntax.svg | 332 +++++++++++++++---------------- 3 files changed, 300 insertions(+), 343 deletions(-) diff --git a/README.md b/README.md index 55abac6..0305ddc 100644 --- a/README.md +++ b/README.md @@ -79,14 +79,19 @@ Each unique placeholder must be defined only once. I.e. for one `{placeholder,St Placeholders with type `DateTime` must have a format specified. The valid values are the names of [the `DateFormat` constructors][dateformat-constructors], e.g. `yMd`, `jms`, or `EEEEE`. -Placeholders with type `num`, `int`, or `double` must also have a format specified. The valid values are the names +Placeholders with type `num`, `int`, or `double` **may have\*** a format specified. The valid values are the names of [the `NumberFormat` constructors][numberformat-constructors], e.g. `decimalPattern`, or `percentPattern`. -In plurals, the `count` placeholder must be of `int` or `num` type. It can be left with no definition, or with just -a `num` type, without the format, the number won't be formatted but simply to-stringed then. [This is Flutter's behavior.][count-placeholder-num-no-format] +In plurals, the `count` placeholder must be of `int` or `num` type. It can be left with no definition. + +Number placeholders without a specified format will be simply `toString()`ed. **Only template files can define placeholders with their type and format.** In non-template languages, placeholders' types and formats are ignored and no logical errors are reported. +> \*If you're using Flutter 3.5 or older, you need to specify format for numeric placeholders. +> Otherwise `flutter gen-l10n` will fail. You can look at the legacy placeholder syntax diagrams +> [for placeholders here][flutter35-placeholders-diagram] and for [plural's `count` placeholders here][flutter35-count-placeholders-diagram]. + #### Examples Below are some examples of strings that make use of placeholders. Simple and well-defined. @@ -182,9 +187,10 @@ git push origin v0.1.1 [releases]: https://github.com/leancodepl/poe2arb/releases [poeditor-tokens]: https://poeditor.com/account/api [term-name-constraint]: https://github.com/flutter/flutter/blob/ce318b7b539e228b806f81b3fa7b33793c2a2685/packages/flutter_tools/lib/src/localizations/gen_l10n.dart#L868-L886 -[count-placeholder-num-no-format]: https://github.com/flutter/flutter/blob/1faa95009e947c66e8139903e11b1866365f282c/packages/flutter_tools/lib/src/localizations/gen_l10n_types.dart#L507-L512 [dateformat-constructors]: https://pub.dev/documentation/intl/latest/intl/DateFormat-class.html#constructors [numberformat-constructors]: https://pub.dev/documentation/intl/latest/intl/NumberFormat-class.html#constructors +[flutter35-placeholders-diagram]: https://github.com/leancodepl/poe2arb/blob/24be17d6721698526c879b3fada87183b359e8e8/art/placeholder-syntax.svg +[flutter35-count-placeholders-diagram]: https://github.com/leancodepl/poe2arb/blob/24be17d6721698526c879b3fada87183b359e8e8/art/count-placeholder-syntax.svg [placeholder-diagram-img]: art/placeholder-syntax.svg [count-placeholder-diagram-img]: art/count-placeholder-syntax.svg [gofumpt]: https://github.com/mvdan/gofumpt diff --git a/art/count-placeholder-syntax.svg b/art/count-placeholder-syntax.svg index 489e124..2abf92a 100644 --- a/art/count-placeholder-syntax.svg +++ b/art/count-placeholder-syntax.svg @@ -1,201 +1,152 @@ - - + + - + - + - - - + + + { - - + + - - - + + + count - + - - - + + + - + - - + + - - + + - - - + + + , - + - - - + + + + + + + + num + + + + + + + + int + + + + + + + - - - - - - - - - - - - - - num - - - - - - - - - - - - - - - - - - - , - - - - - - - - number-format - - - - - - - - - - - - - - - int - - - - - - - - , - - - - - - - - number-format - - - + + + + + + + + + + + + , + + + + + + + + number-format - + - + - + - - - - } + + + + } - - + + + svg { + background-color: hsl(30,20%,95%); + } + path { + stroke-width: 3; + stroke: black; + fill: rgba(0,0,0,0); + } + text { + font: bold 14px monospace; + text-anchor: middle; + white-space: pre; + } + text.diagram-text { + font-size: 12px; + } + text.diagram-arrow { + font-size: 16px; + } + text.label { + text-anchor: start; + } + text.comment { + font: italic 12px monospace; + } + g.non-terminal text { + /*font-style: italic;*/ + } + rect { + stroke-width: 3; + stroke: black; + fill: hsl(120,100%,90%); + } + rect.group-box { + stroke: gray; + stroke-dasharray: 10 5; + fill: none; + } + path.diagram-text { + stroke-width: 3; + stroke: black; + fill: white; + cursor: help; + } + g.diagram-text:hover path.diagram-text { + fill: #eee; + } diff --git a/art/placeholder-syntax.svg b/art/placeholder-syntax.svg index 87938c2..af20f53 100644 --- a/art/placeholder-syntax.svg +++ b/art/placeholder-syntax.svg @@ -1,214 +1,214 @@ - - + + - + - + - - - + + + { - - + + - - - + + + placeholder-name - + - - - + + + - + - - + + - - + + - - - + + + , - + - - - + + + - - - - Object + + + + Object - - + + - - - - String + + + + String - - + + - - + + - - - - DateTime + + + + DateTime - - + + - - - - , + + + + , - - + + - - - - date-time-format + + + + date-time-format - - + + - - + + - - - + + + - - - - int + + + + int - - + + - - - - num + + + + num - - + + - - - - double + + + + double - + - - - - - - , - - - - - - - - number-format + + + + + + + + + + + + + + + + + , + + + + + + + + number-format + + + - + - + - + - - - - } + + + + } - - + + + svg { + background-color: hsl(30,20%,95%); + } + path { + stroke-width: 3; + stroke: black; + fill: rgba(0,0,0,0); + } + text { + font: bold 14px monospace; + text-anchor: middle; + white-space: pre; + } + text.diagram-text { + font-size: 12px; + } + text.diagram-arrow { + font-size: 16px; + } + text.label { + text-anchor: start; + } + text.comment { + font: italic 12px monospace; + } + g.non-terminal text { + /*font-style: italic;*/ + } + rect { + stroke-width: 3; + stroke: black; + fill: hsl(120,100%,90%); + } + rect.group-box { + stroke: gray; + stroke-dasharray: 10 5; + fill: none; + } + path.diagram-text { + stroke-width: 3; + stroke: black; + fill: white; + cursor: help; + } + g.diagram-text:hover path.diagram-text { + fill: #eee; + } From ca0411e20baf1d84e9a859da9f5ea9536cc1fbb6 Mon Sep 17 00:00:00 2001 From: Albert Wolszon Date: Thu, 16 Mar 2023 17:21:36 +0100 Subject: [PATCH 2/3] Make format in numeric placeholders optional, add date test DateTime placeholder with no format had no tests. --- converter/parser.go | 64 +++++++++++++++++++--------------------- converter/parser_test.go | 55 +++++++++++++++++++++++++++------- 2 files changed, 75 insertions(+), 44 deletions(-) diff --git a/converter/parser.go b/converter/parser.go index d6abc5f..d39241d 100644 --- a/converter/parser.go +++ b/converter/parser.go @@ -81,10 +81,11 @@ func (tp *translationParser) Parse(translation string) (string, error) { } func (tp *translationParser) addPlaceholder(name, placeholderType, format string) error { - if placeholder, _ := tp.namedParams.Get(name); placeholder != nil { - // doesn't exist - placeholder was never seen - // exists but nil - placeholder was only seen (used only with name, with no definition) - // exists and not nil - placeholder was defined + if placeholder, present := tp.namedParams.Get(name); placeholder != nil { + _ = present + // present == false - placeholder was never seen + // placeholder == nil - placeholder was only seen (used only with name, with no definition) + // placeholder != nil - placeholder was defined if placeholderType == "" { return nil } else { @@ -93,30 +94,19 @@ func (tp *translationParser) addPlaceholder(name, placeholderType, format string } if tp.plural && name == countPlaceholderName { - if placeholderType == "" { + switch placeholderType { + case "": // filled in by fallbackPlaceholderTypes tp.namedParams.Set(name, nil) return nil - } else if placeholderType == "num" && format == "" { - // Special edge-case, when plural variable doesn't have a type defined, it falls back to num - // and because no actual type in ARB is specified, requires no format. - // https://github.com/flutter/flutter/blob/1faa95009e947c66e8139903e11b1866365f282c/packages/flutter_tools/lib/src/localizations/gen_l10n_types.dart#L507-L512 - tp.namedParams.Set(name, &placeholder{"", ""}) - return nil - } else if placeholderType == "num" { - tp.namedParams.Set(name, &placeholder{"num", format}) + case "num", "int": + tp.namedParams.Set(name, &placeholder{placeholderType, format}) return nil - } else if placeholderType == "int" { - if format == "" { - return errors.New("format is required for int plural placeholders") - } - tp.namedParams.Set(name, &placeholder{"int", format}) - return nil + default: + return errors.New("invalid count placeholder type. Supported types: num, int") } - - return errors.New("unknown placeholder type. Supported types: num, int") } if placeholderType == "" { @@ -125,24 +115,30 @@ func (tp *translationParser) addPlaceholder(name, placeholderType, format string return nil } - if format != "" { - if placeholderType == "DateTime" { - tp.namedParams.Set(name, &placeholder{"DateTime", format}) - return nil - } else if placeholderType == "num" || placeholderType == "int" || placeholderType == "double" { - tp.namedParams.Set(name, &placeholder{placeholderType, format}) - return nil - } else { - return errors.New("format is only supported for DateTime and int, num or double placeholders") + switch placeholderType { + case "num", "int", "double": + tp.namedParams.Set(name, &placeholder{placeholderType, format}) + return nil + + case "String", "Object": + if format != "" { + return fmt.Errorf("format is not supported for %s placeholders", placeholderType) } - } - if placeholderType == "String" || placeholderType == "Object" { tp.namedParams.Set(name, &placeholder{placeholderType, ""}) return nil - } - return errors.New("unknown placeholder type. Supported types: String, Object, DateTime, num, int, double") + case "DateTime": + if format == "" { + return errors.New("format is required for DateTime placeholders") + } + + tp.namedParams.Set(name, &placeholder{"DateTime", format}) + return nil + + default: + return fmt.Errorf("unknown placeholder type %s. Supported types: String, Object, DateTime, num, int, double", placeholderType) + } } func (tp *translationParser) BuildMessageAttributes() *arbMessageAttributes { diff --git a/converter/parser_test.go b/converter/parser_test.go index 3f3c569..e15d79e 100644 --- a/converter/parser_test.go +++ b/converter/parser_test.go @@ -162,14 +162,14 @@ func TestTranslationParserAddPlaceholder(t *testing.T) { Name: "param", Type: "Object", Format: "format", - ExpectedError: "format is only supported for DateTime and int, num or double placeholders", + ExpectedError: "format is not supported for Object placeholders", }, { TestName: "name, type String and format", Name: "param", Type: "String", Format: "format", - ExpectedError: "format is only supported for DateTime and int, num or double placeholders", + ExpectedError: "format is not supported for String placeholders", }, { TestName: "name, type DateTime and format", @@ -180,6 +180,12 @@ func TestTranslationParserAddPlaceholder(t *testing.T) { "param": {"DateTime", "format"}, }, }, + { + TestName: "name, type DateTime and no format", + Name: "param", + Type: "DateTime", + ExpectedError: "format is required for DateTime placeholders", + }, { TestName: "name, type num and format", Name: "param", @@ -207,11 +213,35 @@ func TestTranslationParserAddPlaceholder(t *testing.T) { "param": {"double", "format"}, }, }, + { + TestName: "name, type double and no format", + Name: "param", + Type: "double", + ExpectedPlaceholders: map[string]*placeholder{ + "param": {"double", ""}, + }, + }, + { + TestName: "name, type int and no format", + Name: "param", + Type: "int", + ExpectedPlaceholders: map[string]*placeholder{ + "param": {"int", ""}, + }, + }, + { + TestName: "name, type num and no format", + Name: "param", + Type: "num", + ExpectedPlaceholders: map[string]*placeholder{ + "param": {"num", ""}, + }, + }, { TestName: "name, and invalid type", Name: "param", Type: "invalid", - ExpectedError: "unknown placeholder type. Supported types: String, Object, DateTime, num, int, double", + ExpectedError: "unknown placeholder type invalid. Supported types: String, Object, DateTime, num, int, double", }, { TestName: "name count, but not in plural", @@ -221,12 +251,15 @@ func TestTranslationParserAddPlaceholder(t *testing.T) { "count": {"String", ""}, }, }, + // + // Plural tests + // { TestName: "name count, type String in plural", Plural: true, Name: "count", Type: "String", - ExpectedError: "unknown placeholder type. Supported types: num, int", + ExpectedError: "invalid count placeholder type. Supported types: num, int", }, { TestName: "just name count in plural", @@ -242,7 +275,7 @@ func TestTranslationParserAddPlaceholder(t *testing.T) { Name: "count", Type: "num", ExpectedPlaceholders: map[string]*placeholder{ - "count": {"", ""}, + "count": {"num", ""}, }, }, { @@ -256,11 +289,13 @@ func TestTranslationParserAddPlaceholder(t *testing.T) { }, }, { - TestName: "name count, type int, without format in plural", - Plural: true, - Name: "count", - Type: "int", - ExpectedError: "format is required for int plural placeholders", + TestName: "name count, type int, without format in plural", + Plural: true, + Name: "count", + Type: "int", + ExpectedPlaceholders: map[string]*placeholder{ + "count": {"int", ""}, + }, }, { TestName: "name count, type int, with format in plural", From 52654b3e8dbee647a2d5d8545a11725467162120 Mon Sep 17 00:00:00 2001 From: Albert Wolszon Date: Thu, 16 Mar 2023 17:31:03 +0100 Subject: [PATCH 3/3] Remove trailing newline from error reporting --- converter/parser.go | 4 ++-- converter/parser_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/converter/parser.go b/converter/parser.go index d39241d..86806d8 100644 --- a/converter/parser.go +++ b/converter/parser.go @@ -210,11 +210,11 @@ func (e *translationParserErrors) HasErrors() bool { func (e translationParserErrors) Error() string { var sb strings.Builder - sb.WriteString("some errors occurred while parsing translation:\n") + sb.WriteString("some errors occurred while parsing translation:") for placeholderName, errs := range e.errors { for _, err := range errs { - sb.WriteString(fmt.Sprintf(" - %s: %s\n", placeholderName, err)) + sb.WriteString(fmt.Sprintf("\n - %s: %s", placeholderName, err)) } } diff --git a/converter/parser_test.go b/converter/parser_test.go index e15d79e..ac9edab 100644 --- a/converter/parser_test.go +++ b/converter/parser_test.go @@ -98,7 +98,7 @@ func TestTranslationParserParseErrors(t *testing.T) { { TestName: "placeholder double definitions", Input: "{placeholder,String} with {placeholder,DateTime,yMd}", - ExpectedError: "some errors occurred while parsing translation:\n - placeholder: placeholder type can only be defined once\n", + ExpectedError: "some errors occurred while parsing translation:\n - placeholder: placeholder type can only be defined once", }, }