Repository navigation
refactor generateString to emit simpler dart code - #49334
Conversation
fluttergithubbot
added
a: internationalization
There was a problem hiding this comment.
The previous version didn't emit trailing commas.
c932146 to
05c7ffe
Compare
c198e2a to
40db9e5
Compare
|
/cc @HansMuller |
40db9e5 to
429d217
Compare
429d217 to
5b851f8
Compare
|
@kjlubick this PR has been experiencing some gold failures that look a bit odd. Waiting to see if it happens again. Here is one of the runs that had issues: https://cirrus-ci.com/task/5200111831613440 |
|
That does look strange - the service account that goldctl was authed with ([email protected]) does not look familiar to me. |
|
It looks like it is not recurring with this latest commit. I don't know where that service account would have come from. 🤔 |
|
Thanks for your help. The job that failed happened before I rebase this PR (previously the PR was based on 92f7e16) The rebase seems to have fixed the issue. |
| /// | ||
| /// Strings with newlines are not supported. | ||
| String generateString(String value) { | ||
| assert(!value.contains('\n')); |
There was a problem hiding this comment.
Hi @a14n, could you explain why this newline check was added?
There was a problem hiding this comment.
I think I added this assert because the old method didn't work for s with a \n inside.
There was a problem hiding this comment.
The new method appears to handle any backslash character by generating a raw string. Maybe this assert isn't really needed.
There was a problem hiding this comment.
The assert is currently needed because the method does not handle \n. We could change it to use triple quote if s contains \n but it's a different handling than \\
There was a problem hiding this comment.
You're right; it's newline characters that are disallowed, not a backslash followed by an 'n'.
Values in JSON files - and ARB files are JSON files - cannot contain newline characters, so it makes sense that this method doesn't allow values to contain newline characters.
| return "r'''$json'''"; | ||
| } | ||
| if (json is String) | ||
| return generateEncodedString(currentLocale, json); |
There was a problem hiding this comment.
@a14n A separate question, but this function was originally created for use to handle the Kannada locale. What's the rationale for changing its purpose here? It also seems like the documentation for the function have not been appropriately updated so I'm not quite sure what the intent is
There was a problem hiding this comment.
generateEncodedString now manages both Kannada and not-Kannada. The test on locale is done at a single place.
It also seems like the documentation for the function have not been appropriately updated
Indeed it should now mention that the method handles both Kannada and not-Kannada. Perhaps a better function name could be found.
|
This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of |
7 participants
Description
In #49159 we try to handle whitepaces in adjacent strings. Some of the lints were in generated files.
This PR changes how we output dart String in generated code to have more terse and idiomatic Strings.
Related Issues
None
Tests
I added several tests for
generateStringfunction.Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.