CLDR-13090 Added a helper function to obtain Attribute instances efficiently. #48
Conversation
|
|
hagbard
changed the title
hagbard
changed the title
hagbard
changed the title
|
BTW, cleaning up the formatting at the same time as making substantive changes to items is confusing for review. Please do one or the other in a PR, not both. |
|
Yeah, I'd only just realised that my editor was stripping EOL whitespace. I'll undo that!! |
|
Right, hopefully that's sorted it. Also, I'm not sure what to do about the error: |
|
Change looks good. The process requires a ticket first before merging (see the above), and that ticket will need to be approved. If you file the ticket, I can send around to people to approve. |
|
https://unicode-org.atlassian.net/projects/CLDR/issues/CLDR-13090 |
hagbard
changed the title
|
Yes, the PR is checked to make sure that the commit has the right format,
referencing the ticket.
Mark
…On Fri, Jun 7, 2019 at 5:42 PM David Beaumont ***@***.***> wrote:
https://unicode-org.atlassian.net/projects/CLDR/issues/CLDR-13090
Should I update the title in this pull request?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#48?email_source=notifications&email_token=ACJLEMDIQEZ2P6IYEJPQJWTPZJ6UZA5CNFSM4HVWW2X2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXGGSNA#issuecomment-499935540>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACJLEMGN3GUJFCROHMZA4OTPZJ6UZANCNFSM4HVWW2XQ>
.
|
|
Looks like the wrong commit message was used. See the details on the ticket check. |
|
Hmm, so how do I change the commit message? I cannot see a way to edit it. |
|
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
|
It happens when there is a commit that doesn't have the ticket number at the start of the commit message. There's a "Details" link on the check that lets you squash with new message. Looks like you fixed that, so it should go forward. |
|
hmm, so now it's complaining I'm not "authorized" to merge this :( |
|
I'm merging, since David doesn't have access. |
This should be one of only a small number of changes to existing code required to support the new CLDR data API.
I'm not necessarily seeking a merge at this stage, but just putting the expected changes out for discussion. The prototype API and some unit tests will be added separately (or if people want I can combine into a single pull request).
It turned out to be effectively impossible to simply isolate existing methods and classes to move/expose for a public API, and what I've ended up doing is create a very minimal new API in a new package (but using lots of the existing CLDR classes behind the scenes). This means that there won't be much changing with existing code, which is nice because it removes any risk of breaking things while the new API and ICU tools are being converted to the new API.