Fix area1 formatting rules for DE, FR, NL and PL#87
Conversation
| @@ -1,5 +1,12 @@ | |||
| formatting-rules: | |||
| address: | |||
| # {building-location} | |||
There was a problem hiding this comment.
Please remove these sort of comments. They aren't adding much.
There was a problem hiding this comment.
They are very helpful for my understanding.
Are they hurting your understanding? Otherwise I'd keep.
There was a problem hiding this comment.
Yes, it's not at all clear to me what they are adding. Let's remove them.
| - street-address-alternative-1 | ||
| - separator: "\n" | ||
| - locality1 | ||
| - separator: " " |
There was a problem hiding this comment.
Why are these separator dropped?
There was a problem hiding this comment.
This is more of a cosmetic change.
Since the "single space" separator is default, I think we should consistently use it or not use it (it differed between files before)
There was a problem hiding this comment.
(Also the generated cpp files have no diff).
There was a problem hiding this comment.
I think even if a single space is the default separator I believe they add clarity. Let's keep them.
There was a problem hiding this comment.
Sounds good.
I'll create a separate PR that adds single spaces where needed so that we have a consistent behavior (single spaces everywhere; default not used)
There was a problem hiding this comment.
Actually I agree that they add clarity. Let me add them everywhere here already (since the discussion is happening in this PR)
| - separator: "\n" | ||
| - country-name | ||
| - skip: country # redundant with country-name | ||
| - skip: street-address # redundant with street-address-alternative-1 |
There was a problem hiding this comment.
this is dropped, since it's confusing and pointless. We shouldn't skip the field that is included.
There was a problem hiding this comment.
Are you certain that street-address is not generated if it is not explicitly skipped? IIRC, that was the main reason why these skip rules were included.
There was a problem hiding this comment.
this is needed for other locales, where there is a country-specific model that includes street-address-alternative-1. There we need to skip street-address not to have 2 nodes that represent ADDRESS_HOME_STREET_ADDRESS.
For Canada (and the US for example) we use street-address.
I guess what happens when street-address is both listed and skipped is an undefined behavior, and that just happens to be no op, but it's extremely confusing.
There was a problem hiding this comment.
Both street-address and street-address-alternative-1 map to ADDRESS_HOME_STREET_ADDRESS. If US and CA are both using street-address, shouldn't we then skip street-address-alternative-1? Otherwise IIUC we will be generating ADDRESS_HOME_STREET_ADDRESS twice for US and CA.
There was a problem hiding this comment.
We don't have to skip it in the formatting rules, the child (street-address-alternative-1) is already cut-off in the CA-model.yaml and US-model.yaml.
We theoretically can add the "skip street-address-alternative-1" but it's not needed.
I compared the generated c++ files and adding/removing this skip is no-op
There was a problem hiding this comment.
Thanks for checking. Let's add then skip street-address-alternative-1 for consistency and we should be good to go.
There was a problem hiding this comment.
ahh sorry for the fake info ;)
I tried this last week and I forgot the outcome.
Actually adding this skip didn't work:
error message:
"
Formatting errors for address: ["'skip street-address-alternative-1' exists in the rule but is not a descendant of 'address' in the model."]
address: [street-address: [address-line1, address-line2, address-line3, address-line4], locality1, admin-area1, postal-code: [postal-code-prefix, postal-code-suffix], country, country-name]
"
this is expected per my previous comment.
| - street-address-alternative-1 | ||
| - separator: "\n" | ||
| - postal-code | ||
| - separator: " " |
There was a problem hiding this comment.
Why is this separator dropped? Similar question on the files below.
This was autogenerated after changed introduced by battre/autocomplete-attribute-explainer#87. This adds ADDRESS_HOME_STATE (Land, Région, provincies, województwo) to parsing rules for these countries as a separate line on top of country. After this change all the countries' admin-area1 are includes in proper formatting rules. This brings us closer to achieve growth invariant Bug: 450838894 Change-Id: I187d4c3f81d079239e3e605c8faf81d40e7d11cd Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7474650 Commit-Queue: Przemek Perkowski <perkowski@google.com> Reviewed-by: Norge Vizcay <vizcay@google.com> Reviewed-by: Karol Sygiet <sygiet@google.com> Cr-Commit-Position: refs/heads/main@{#1580711}
This is a changed version of #84
This also performs accompanying cleanups and updates the generated HTML files.