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 #3189: Create build bazel for each subdirectories of util parser new branch #3915

Conversation

yash10019coder
Copy link
Contributor

@yash10019coder yash10019coder commented Oct 8, 2021

Fixes #3189

Explanation

Created the BUILD bazel for the subdirectories of the util/parser I have also separated ImageTransformation and ImageLoader as they were giving cycle error with this PR we'll move towards the goal of Gradle to Bazel migration

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

If your PR includes UI-related changes, then:

  • Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
  • For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
  • Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
  • Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

@BenHenning BenHenning self-assigned this Oct 8, 2021
@BenHenning
Copy link
Sponsor Member

Thanks @yash10019coder. Apologies, but I'll need to take a look at this on Monday.

In the meantime, I suggest taking a look at the CI failures & fixing them.

@yash10019coder
Copy link
Contributor Author

@FareesHussain made a new commit and getting this error please help
image
earlier before the commit this error
image

@FareesHussain
Copy link
Contributor

@yash10019coder You need to make changes as mentioned in the error message

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! Gave it a first pass.

Another point: no dependencies elsewhere in the app are being updated to use these new ones (which doesn't seem right since, by adding the BUILD.bazel files, these will automatically be excluded from //utility:utility). Please update other libraries & test deps in the app to point to these new libraries, as needed.

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.

adresed all the comments

@yash10019coder
Copy link
Contributor Author

yash10019coder commented Oct 17, 2021

Hi @BenHenning I have done almost all comments only two are left ig glide one and commented code one
I am having a build error therefore im unable to resolve the glide comment now and
for the commented code i have merged four targets into one target as they were having a cycle in dependency graph
please help with this error
image
thanks

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.

Hi @BenHenning I have done almost all comments only two are left ig glide one and commented code one I am having a build error therefore im unable to resolve the glide comment now and for the commented code i have merged four targets into one target as they were having a cycle in dependency graph please help with this error image thanks

Can you create a debugging document (per https://github.com/oppia/oppia/wiki/Debugging-Docs) that outlines the different issues that you ran into, what led to those issues, and what you've tried? It's hard for me to provide feedback given the current information; a debugging doc would really help contextualize the problems you're hitting better so that I can give better feedback.

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 I have adressed both the comments and also created the debugging doc thanks

@BenHenning
Copy link
Sponsor Member

Sorry, will need to review this tomorrow.

Thanks @yash10019coder. I'll follow up on the doc tomorrow (my time).

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 mentioned all the comments and also added some comments in the code justifying the visibility of the targets PTAL.
Thanks

domain/BUILD.bazel Show resolved Hide resolved
domain/BUILD.bazel Show resolved Hide resolved
domain/BUILD.bazel Outdated Show resolved Hide resolved
domain/BUILD.bazel Outdated Show resolved Hide resolved
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 few comments left, otherwise the PR LGTM!

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 mentioned all the comments and also resolved all the comments where you have mentioned "sounds good" PTAL thanks.

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 one comment left to resolve, I think.

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 made the requested changes PTAL thanks.

@yash10019coder
Copy link
Contributor Author

Thanks @yash10019coder. Just one comment left to resolve, I think.

done

@BenHenning
Copy link
Sponsor Member

Hi. As of today, some main reviewers have taken time off for the next few weeks, so it may take a little while before we can look at this PR. We appreciate your patience while some of our team members recharge. We'll be fully returning on 4 January 2021.

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! LGTM. This was a gnarly PR, so thanks for your help.

@yash10019coder
Copy link
Contributor Author

Hi @BenHenning thanks a lot for your reply merry christmas.

@oppiabot
Copy link

oppiabot bot commented Dec 17, 2021

Unassigning @BenHenning since they have already approved the PR.

@BenHenning
Copy link
Sponsor Member

Merging since all CI tests are passing.

@BenHenning BenHenning merged commit 6c54f39 into oppia:develop Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a BUILD.bazel file in each of the subdirectories of util/parser
3 participants