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

Fix #3486: Call out dependency names when license details incomplete Exception is thrown #3503

Merged
merged 11 commits into from
Jul 30, 2021
Merged

Fix #3486: Call out dependency names when license details incomplete Exception is thrown #3503

merged 11 commits into from
Jul 30, 2021

Conversation

prayutsu
Copy link
Contributor

@prayutsu prayutsu commented Jul 19, 2021 •
edited

Explanation

Fixes #3486:

When an exception is thrown to update license details, the script now calls out the specific dependencies that should be updated to update all other occurrences of those licenses.

Screenshot of passing test cases -

Screenshot from 2021-07-22 12-36-36

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

@prayutsu prayutsu requested a review from rt4914 July 19, 2021 11:28
@prayutsu
Copy link
Contributor Author

@BenHenning @rt4914 PTAL.

rt4914
rt4914 suggested changes Jul 20, 2021
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

@prayutsu Suggested nit changes.

third_party/versions.bzl Outdated Show resolved Hide resolved
scripts/src/java/org/oppia/android/scripts/maven/GenerateMavenDependenciesList.kt Outdated Show resolved Hide resolved
scripts/src/javatests/org/oppia/android/scripts/maven/GenerateMavenDependenciesListTest.kt Outdated Show resolved Hide resolved
@rt4914 rt4914 removed their assignment Jul 20, 2021
@oppiabot
Copy link

oppiabot bot commented Jul 20, 2021

Hi @prayutsu, it looks like some changes were requested on this pull request by @rt4914. PTAL. Thanks!

BenHenning
BenHenning requested changes Jul 20, 2021
View reviewed changes
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @prayutsu! Left some thoughts.

scripts/src/java/org/oppia/android/scripts/maven/GenerateMavenDependenciesList.kt Outdated Show resolved Hide resolved
scripts/src/java/org/oppia/android/scripts/maven/GenerateMavenDependenciesList.kt Show resolved Hide resolved
scripts/src/java/org/oppia/android/scripts/maven/GenerateMavenDependenciesList.kt Show resolved Hide resolved
scripts/src/java/org/oppia/android/scripts/maven/GenerateMavenDependenciesList.kt Outdated Show resolved Hide resolved
scripts/src/java/org/oppia/android/scripts/maven/GenerateMavenDependenciesList.kt Outdated Show resolved Hide resolved
scripts/src/java/org/oppia/android/scripts/maven/GenerateMavenDependenciesList.kt Outdated Show resolved Hide resolved
scripts/src/java/org/oppia/android/scripts/maven/GenerateMavenDependenciesList.kt Outdated Show resolved Hide resolved
scripts/src/java/org/oppia/android/scripts/maven/GenerateMavenDependenciesList.kt Outdated Show resolved Hide resolved
scripts/src/javatests/org/oppia/android/scripts/maven/GenerateMavenDependenciesListTest.kt Outdated Show resolved Hide resolved
third_party/versions.bzl Outdated Show resolved Hide resolved
@BenHenning BenHenning assigned prayutsu and unassigned BenHenning and prayutsu Jul 20, 2021
@prayutsu prayutsu assigned BenHenning and rt4914 and unassigned prayutsu Jul 21, 2021
@prayutsu
Copy link
Contributor Author

@BenHenning @rt4914 PTAL

BenHenning
BenHenning reviewed Jul 22, 2021
View reviewed changes
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @prayutsu. Left some more thoughts--PTAL.

scripts/src/javatests/org/oppia/android/scripts/maven/GenerateMavenDependenciesListTest.kt Outdated Show resolved Hide resolved
scripts/src/javatests/org/oppia/android/scripts/maven/GenerateMavenDependenciesListTest.kt Outdated Show resolved Hide resolved
scripts/src/javatests/org/oppia/android/scripts/maven/GenerateMavenDependenciesListTest.kt Outdated Show resolved Hide resolved
scripts/src/java/org/oppia/android/scripts/maven/GenerateMavenDependenciesList.kt Outdated Show resolved Hide resolved
@BenHenning BenHenning assigned prayutsu and unassigned BenHenning Jul 22, 2021
@BenHenning
Copy link
Sponsor Member

Apologies. I don't think I'm going to be able to look at this before the end of the weekend, though my follow-up review will probably be faster once @rt4914 has a chance to take a full pass on this after the latest changes.

@prayutsu
Merge branches 'callout-coordinate-name' and 'develop' of github.com:…
5d39b7f 
…oppia/oppia-android into callout-coordinate-name
@prayutsu prayutsu removed their assignment Jul 24, 2021
rt4914
rt4914 approved these changes Jul 25, 2021
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @prayutsu

@rt4914 rt4914 removed their assignment Jul 25, 2021
BenHenning
BenHenning requested changes Jul 28, 2021
View reviewed changes
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @prayutsu. Generally LGTM, just had one comment leftover from earlier that I followed up on. PTAL.

@BenHenning BenHenning assigned prayutsu and unassigned BenHenning Jul 28, 2021
@prayutsu prayutsu assigned BenHenning and unassigned BenHenning Jul 28, 2021
@prayutsu
Copy link
Contributor Author

@BenHenning Made the suggested changes, PTAL.

@oppiabot oppiabot bot assigned BenHenning and unassigned prayutsu Jul 28, 2021
@oppiabot
Copy link

oppiabot bot commented Jul 28, 2021

Unassigning @prayutsu since a re-review was requested. @prayutsu, please make sure you have addressed all review comments. Thanks!

BenHenning
BenHenning approved these changes Jul 30, 2021
View reviewed changes
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @prayutsu! LGTM.

@BenHenning
Copy link
Sponsor Member

BenHenning commented Jul 30, 2021 •
edited

The single Bazel test failure seems like a transient issue unrelated to this PR, and we don't currently have a way to restart individual Bazel tests. I think it's fine to merge this PR as-is. All approvals have been given & there are no other open conversation threads.

@BenHenning BenHenning merged commit 24ff6ec into develop Jul 30, 2021
@BenHenning BenHenning deleted the callout-coordinate-name branch July 30, 2021 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rt4914 rt4914 rt4914 approved these changes

@BenHenning BenHenning BenHenning approved these changes

Assignees

@BenHenning BenHenning

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@prayutsu @BenHenning @rt4914