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.

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

Fix #3332: Created a BUILD.bazel file in domain/audio #3967

Merged

Conversation

yash10019coder
Copy link
Contributor

@yash10019coder yash10019coder commented Oct 24, 2021

Explanation

Fixes #3332
Created the bazel file in audio and also added targets to the needed targets.

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

N/A -- this PR is only changing the Bazel build graph and doesn't affect any user flows.

Copy link
Contributor Author

@yash10019coder yash10019coder left a comment

Choose a reason for hiding this comment

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

Hi @BenHenning created a PR PTAL

@BenHenning
Copy link
Sponsor Member

Thanks @yash10019coder. Apologies, will need to look at this tomorrow.

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 @yash10019coder. Just had a couple of comments, otherwise the PR looks quite good to me.

Regarding the PR itself, three things:

  1. Could you please update the title? It's currently cut-off. Titles should generally be concise and specific as to the high-level thing the PR is accomplishing.
  2. Could you please move the explanation portion in your PR description to be under the 'explanation' section?
  3. You can (& should) remove the bits under "For UI-specific PRs only" and describe why this PR doesn't need that (e.g. "N/A -- this PR is only changing the Bazel build graph and doesn't affect any user flows"). In general, please make sure to follow the instructions in the PR description to ensure that all the needed pieces are included. This really helps both reviewers and future people who come across the PR after it gets merged.

domain/BUILD.bazel Outdated Show resolved Hide resolved
Co-authored-by: Ben Henning <henning.benmax@gmail.com>
@yash10019coder yash10019coder changed the title Fix #3332: created the targets in the audio/bazel and also added the targets to … Fix #3332: Created a BUILD.bazel file in domain/audio Oct 27, 2021
Copy link
Contributor Author

@yash10019coder yash10019coder left a comment

Choose a reason for hiding this comment

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

@BenHenning Please PTAL

Copy link
Contributor Author

@yash10019coder yash10019coder left a comment

Choose a reason for hiding this comment

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

Hi @BenHenning added the dependencies and removed the old dependencies from domian/bazel PTAL

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 @yash10019coder! The PR LGTM.

Two things:

  1. I updated your PR description since you didn't address point (3) in my earlier comment. Please see how I changed it for a reference on future PRs--copying that approach will help streamline future PRs that you send.
  2. Please remember to reply to comment threads directly when they're addressed

@BenHenning BenHenning merged commit c3e3c02 into oppia:develop Oct 30, 2021
@yash10019coder
Copy link
Contributor Author

Thanks a lot, @BenHenning for merging this PR 😄

@yash10019coder
Copy link
Contributor Author

Thanks @yash10019coder! The PR LGTM.

Two things:

  1. I updated your PR description since you didn't address point (3) in my earlier comment. Please see how I changed it for a reference on future PRs--copying that approach will help streamline future PRs that you send.
  2. Please remember to reply to comment threads directly when they're addressed

apologies for not replying I forgot I will always reply in the future for all the comments
thanks

@BenHenning
Copy link
Sponsor Member

Thanks @yash10019coder! The PR LGTM.
Two things:

  1. I updated your PR description since you didn't address point (3) in my earlier comment. Please see how I changed it for a reference on future PRs--copying that approach will help streamline future PRs that you send.
  2. Please remember to reply to comment threads directly when they're addressed

apologies for not replying I forgot I will always reply in the future for all the comments thanks

Thanks @yash10019coder, I appreciate it. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a BUILD.bazel file in domain/audio [Blocked by #3324]
2 participants