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.

Sign up for GitHub

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

Jump to bottom

CLDR-17137 Add --license-file to Ldml2json for overriding the license included in the binary #3300

Merged
merged 1 commit into from
Oct 4, 2023
Merged

CLDR-17137 Add --license-file to Ldml2json for overriding the license included in the binary #3300

merged 1 commit into from
Oct 4, 2023

Conversation

Manishearth
Copy link
Member

CLDR-17137

  • This PR completes the ticket.

ALLOW_MANY_COMMITS=true

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • tools/cldr-code/src/main/java/org/unicode/cldr/json/Ldml2JsonConverter.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • tools/cldr-code/src/main/java/org/unicode/cldr/json/Ldml2JsonConverter.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

srl295
srl295 requested changes Oct 3, 2023
View reviewed changes
Copy link
Member

@srl295 srl295 left a comment

Choose a reason for hiding this comment

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

I’d prefer --license-file= - there should always be some kind of license file.

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • tools/cldr-code/src/main/java/org/unicode/cldr/json/Ldml2JsonConverter.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@Manishearth Manishearth changed the title CLDR-17137 Add --skip-license to Ldml2Json CLDR-17137 Add --license-file to Ldml2json for overriding the license included in the binary Oct 4, 2023
@Manishearth Manishearth requested a review from srl295 October 4, 2023 20:00
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • tools/cldr-code/src/main/java/org/unicode/cldr/json/Ldml2JsonConverter.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • tools/cldr-code/src/main/java/org/unicode/cldr/json/Ldml2JsonConverter.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

srl295
srl295 previously approved these changes Oct 4, 2023
View reviewed changes
Copy link
Member

@srl295 srl295 left a comment

Choose a reason for hiding this comment

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

LGTM

Need to run mvn spotless:apply

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • tools/cldr-code/src/main/java/org/unicode/cldr/json/Ldml2JsonConverter.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@Manishearth Manishearth requested a review from srl295 October 4, 2023 21:06
@srl295 srl295 requested a review from btangmu October 4, 2023 21:34
@srl295
Copy link
Member

srl295 commented Oct 4, 2023

FYI @btangmu

@Manishearth are you able to merge it yourself?

@Manishearth
Copy link
Member Author

Nope!

@srl295 srl295 merged commit 076d4f5 into unicode-org:main Oct 4, 2023
7 checks passed
@Manishearth Manishearth deleted the skiplicense branch October 4, 2023 21:39
@Manishearth
Copy link
Member Author

Thanks!

srl295
srl295 reviewed Oct 23, 2023
View reviewed changes
tools/cldr-code/src/main/java/org/unicode/cldr/json/Ldml2JsonConverter.java
FileCopier.copy(br, outf);
}
} else {
FileCopier.copy(CldrUtility.getUTF8Data("unicode-license.txt"), outf);
Copy link
Member

Choose a reason for hiding this comment

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

I didn't get to actually run this before.. but now that I am, it's failing. Why was the name unicode-license.txt used here? That's actually not in the build system, and hasn't been for some time. It should have kept the name LICENSE. I'll reopen this and make a PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's because the place I tested this had unicode-license.txt. I guess it's an old holdover.

srl295 added a commit to srl295/cldr that referenced this pull request Oct 23, 2023
@srl295
CLDR-17137 json: fix LICENSE file
e8d24ad 
- code reading LICENSE file was inadvertently using an obsolete filename

See: 076d4f5 unicode-org#3300
srl295
srl295 reviewed Oct 23, 2023
View reviewed changes
tools/cldr-code/src/main/java/org/unicode/cldr/json/Ldml2JsonConverter.java
// not be available as it is put in place by pom.xml
.add(
"license-file",
null,
Copy link
Member

Choose a reason for hiding this comment

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

null might not be right here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srl295 srl295 srl295 approved these changes

@btangmu btangmu Awaiting requested review from btangmu

Assignees

@srl295 srl295

@Manishearth Manishearth

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Manishearth @srl295