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 #1908:Changed comments in Proto files #1913

Closed
wants to merge 5 commits into from
Closed

Fix #1908:Changed comments in Proto files #1913

wants to merge 5 commits into from

Conversation

Victor-Titan
Copy link
Contributor

@Victor-Titan Victor-Titan commented Sep 27, 2020

Explanation

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.

PTAL @prayutsu

@oppiabot
Copy link

oppiabot bot commented Sep 27, 2020

Hi! @Victor-Titan, Welcome to Oppia! Please could you follow the instructions here to get started? You'll need to do this before we can accept your PR. I am closing this PR for now. Feel free to re-open it once you are done with the above instructions. Thanks!

@oppiabot oppiabot bot closed this Sep 27, 2020
@Sarthak2601 Sarthak2601 reopened this Sep 28, 2020
@Sarthak2601
Copy link
Contributor

Hey @Victor-Titan! Thanks for contributing to Oppia :)
There are a couple of things that you can do to improve your pull requests -

  • Follow this guide.
  • Always claim the issue first (by commenting on it or by self assigning) and then open a PR corresponding to that issue. Right now, the issue that you've opened up your PR for, has been assigned to someone else.
  • The title of you PR should be - Fix #1908: Changed comments in Proto file.

Happy contributing 👍

@Victor-Titan Victor-Titan changed the title Fixed #1876 Fix #1876:Changed comments in Proto files Sep 28, 2020
@Victor-Titan Victor-Titan changed the title Fix #1876:Changed comments in Proto files Fix #1908:Changed comments in Proto files Sep 28, 2020
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.

Thanks @Victor-Titan
Added a few points to check on. Let me know if you need any help on these.

.idea/codeStyles/Project.xml Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
Copy link
Contributor

@Sarthak2601 Sarthak2601 left a comment

Choose a reason for hiding this comment

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

Thanks @Victor-Titan. Suggested just one change.

@@ -161,4 +161,4 @@
</indentOptions>
</codeStyleSettings>
</code_scheme>
</component>
</component>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't push .idea file changes. You can simply delete this file from your PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I didn't know we could delete that, else I would've just done that instead of changing the code

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, this is incorrect actually. What you've done might lead to deleting of the file from our codebase itself. I'll defer to @anandwana001 on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the steps you should take here is:

  1. Update your branch with the latest develop branch. (pull + push or fetch + merge + commit + push)
  2. Now, the .idea/codeStyles/Project.xml will be added to your branch, as coming from the latest develop branch.
  3. Now, you will see the file is same as this file https://github.com/oppia/oppia-android/blob/develop/.idea/codeStyles/Project.xml and no changes will be shown into your branch, meaning Android Studio is not updating this file.
  4. Now, in this PR you will only see 3 files changed which is our proto file where you had solved this issue.

As per @Sarthak2601 deleting this file from your pr means you can rollback the changes in this file and then commit.

@Victor-Titan
Copy link
Contributor Author

PTAL @Sarthak2601 @anandwana001 I've implemented the changes you asked

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.

We don't need to delete .idea/codeStyles/Project.xml but keep it unchanged.
Merge your branch with the latest develop branch.

@Victor-Titan
Copy link
Contributor Author

I pulled the code from the main repo and commited again, but the tests showed a build fail in the part of the code I pulled. If there was a build fail issue then how was that code merged to develop? PTAL @anandwana001

@anandwana001
Copy link
Contributor

I pulled the code from the main repo and commited again, but the tests showed a build fail in the part of the code I pulled. If there was a build fail issue then how was that code merged to develop? PTAL @anandwana001

This particular test is passing for me. And also passing on the latest develop branch. Let's just first clear the .idea/codeStyles/Project.xml issue we are having in this PR and then we can see if the test fails or not, It's not dependent on that particularly but then we can update the branch again. Also, force push is not much recommended.

@aggarwalpulkit596
Copy link
Contributor

@Victor-Titan hey victor let's do one thing let's clean up your fork first. For that you can do things either delete the existing one and make new one or add a remote

@prayutsu
Copy link
Contributor

@Victor-Titan Have you installed the Kotlin-plugin in your Android Studio, if you want to fix that Project.xml issue, you can uninstall it, it's not of much use. This bug is actually shipped with the latest version of the Kotlin Plug-in.

@prayutsu
Copy link
Contributor

prayutsu commented Sep 29, 2020

I pulled the code from the main repo and commited again, but the tests showed a build fail in the part of the code I pulled. If there was a build fail issue then how was that code merged to develop? PTAL @anandwana001

This problem has been experienced by a lot of developers recently, and as for now the best solution to avoid this is to uninstall the Kotlin Plug-in, if you haven't installed Kotlin Plug-in then let me know, I'll suggest another way to fix it.

@Victor-Titan
Copy link
Contributor Author

@prayutsu @aggarwalpulkit596 @anandwana001 I removed the Kotlin plugin and also deleted my existing fork and made a new one, but when I went ahead with committing, the .idea/codeStyles/Project.xml file still got changed. Please help. While I can remove that file while committing code, doing so everytime seems tedient

@aggarwalpulkit596
Copy link
Contributor

aggarwalpulkit596 commented Sep 29, 2020

This PR should be closed in that case ? And it won't be pushed for sure for a new fork I'm pretty sure about that this entire folder has been added to gitignore so that case shouldn't be there

@prayutsu
Copy link
Contributor

prayutsu commented Sep 29, 2020

@prayutsu @aggarwalpulkit596 @anandwana001 I removed the Kotlin plugin and also deleted my existing fork and made a new one, but when I went ahead with committing, the .idea/codeStyles/Project.xml file still got changed. Please help. While I can remove that file while committing code, doing so everytime seems tedient

Okay

I want you to confirm one thing, if you have successfully uninstalled Kotlin plug-in, then please try deleting that extra piece of code one last time and try committing changes.

If it still appears again then please follow the steps -

You have to restore your Android Studio Settings to default. Assuming that you are on windows, I
am sharing a link to do this.

https://stackoverflow.com/questions/20582577/how-to-reset-to-default-configuration-in-android-studio/31228003

If you are on linux or mac then do let me know.

Also after doing this don't forget to delete the extra code that is getting appeared again and again. After deleting one last time, you should not encounter this error.

@prayutsu
Copy link
Contributor

Also, if you can please share the Screenshot to confirm that you have successfully uninstalled the Kotlin plug-in, because I am quite sure that this problem comes with the latest release of Kotlin plug-in.

@prayutsu
Copy link
Contributor

@Victor-Titan Also, Android Studio will prompt you again and again by showing the dialog bubble in the bottom right corner but do not install the Kotlin Plugin for now.

@Victor-Titan
Copy link
Contributor Author

@prayutsu I can't seem to uninstall the Kotlin Plugin, I'm only allowed to disable it. When I tried disabling it, a message popped up showing that it was part of a bundle of other google plugins including Firebase(I've not installed any of them, I think they were there to start with) and when I disabled them all and restarted, I got a message like this
image

I'm on Linux. Also, there was another Kotlin Plugin which I had installed back when I started Android dev in Kotlin. I think I uninstalled that one.

@Victor-Titan
Copy link
Contributor Author

@prayutsu @anandwana001 I'm closing this PR as I created a new PR when I pushed code to a new fork as @aggarwalpulkit596 suggested.

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.

Fix comments broken in #1876
6 participants