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

Updated .gitignore and automated setup procedure #3943

Merged
merged 10 commits into from
Nov 10, 2021

Conversation

FareesHussain
Copy link
Contributor

@FareesHussain FareesHussain commented Oct 16, 2021

Updated scripts for setup.sh to automatically update the local.properties for M1 mac (to use protoc using gradle)
Added misc.xml and runconfigurations.xml to .gitignore

scripts/setup.sh Outdated
arch_name="$(uname -m)"
if [ "${arch_name}" = "x86_64" ]; then
if [ "$(sysctl -in sysctl.proc_translated)" = "1" ]; then
echo '\nprotobuf_platform=osx-x86_64' >> ../oppia-android/local.properties
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#3891 for context

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Suggest adding this context as a comment if it's important to make the association (it's more important to have that as a permanent record in code vs. a PR comment that might be hard to find in the future).

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

It seems my comment is no longer relevant--the latest version of this is now using a shell script.

@FareesHussain FareesHussain changed the title Improved contributor Experience Updated .gitignore and automated setup procedure Oct 16, 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 @FareesHussain! Just had one comment--PTAL.

.gitignore Outdated Show resolved Hide resolved
@BenHenning
Copy link
Sponsor Member

Sorry, will need to review this tomorrow.

Copy link
Contributor

@vinitamurthi vinitamurthi 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 Farees!

@oppiabot
Copy link

oppiabot bot commented Oct 20, 2021

Unassigning @vinitamurthi since they have already approved the PR.

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.

Apologies; I probably could have followed up yesterday (I didn't realize you were just looking for clarification). PTAL @FareesHussain.

.gitignore Outdated Show resolved Hide resolved
scripts/setup.sh Outdated Show resolved Hide resolved
@oppiabot
Copy link

oppiabot bot commented Oct 23, 2021

Unassigning @anandwana001 since the review is done.

@oppiabot
Copy link

oppiabot bot commented Oct 25, 2021

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

@anandwana001
Copy link
Contributor

Got it, PTAL at my one last comment for comment update, else all LGTM

@FareesHussain
Copy link
Contributor Author

Got it, PTAL at my one last comment for comment update, else all LGTM

@anandwana001 can you resolve the other comments I'm unable to understand which one (all of them are related to comment)

@oppiabot oppiabot bot assigned anandwana001 and unassigned FareesHussain Oct 25, 2021
@oppiabot
Copy link

oppiabot bot commented Oct 25, 2021

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

Co-authored-by: Akshay Nandwana <akshaynandwana001@gmail.com>
Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

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

LGTM

@oppiabot
Copy link

oppiabot bot commented Oct 26, 2021

Unassigning @anandwana001 since they have already approved the PR.

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 @FareesHussain. Just had two more comments, PTAL.

.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@oppiabot
Copy link

oppiabot bot commented Nov 5, 2021

Hi @FareesHussain, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Nov 5, 2021
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Nov 10, 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 @FareesHussain. I think this looks great! Approved for codeowners.

scripts/setup.sh Outdated
arch_name="$(uname -m)"
if [ "${arch_name}" = "x86_64" ]; then
if [ "$(sysctl -in sysctl.proc_translated)" = "1" ]; then
echo '\nprotobuf_platform=osx-x86_64' >> ../oppia-android/local.properties
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

It seems my comment is no longer relevant--the latest version of this is now using a shell script.

@BenHenning
Copy link
Sponsor Member

Merging since there don't seem to be any outstanding comments to address, CI checks are passing, and this should be a big help with contributor PRs (since we keep seeing people changing unrelated .idea files).

@BenHenning BenHenning merged commit 7465b0b into oppia:develop Nov 10, 2021
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.

None yet

4 participants