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

Removed all code related to sign in through google from app. #3333

Merged
merged 7 commits into from
Jul 12, 2024

Conversation

ahsanishaq721
Copy link
Contributor

@ahsanishaq721 ahsanishaq721 commented Jul 11, 2024

…ts and files.

Pull request (PR) checklist

Please check if your pull request fulfills the following requirements:

  • The PR is submitted to the main branch.
  • I've read the Contribution Guidelines and my PR doesn't break the rules.
  • I've read and understand the Developer Guidelines.
  • I confirm that I've run the code locally and everything works as expected.
  • My PR includes only the necessary changes to fix the issue (i.e., no unnecessary files or lines of code are changed).
  • 🎬 I've attached a screen recording of using the new code to the next paragraph (if applicable).

Screen recording of interacting with your changes:

What's changed?

Describe with a few bullets what's new:

  • ...

  • ...

Risk factors

What may go wrong if we merge your PR?

  • ...
  • ...

In what cases won't your code work?

  • ...
  • ...

Does this PR close any GitHub issues?

Troubleshooting CI failures ❌

Pull request checks failing? Read our CI Troubleshooting guide.

Copy link
Collaborator

@ILIYANGERMANOV ILIYANGERMANOV left a comment

Choose a reason for hiding this comment

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

@ahsanishaq721 fix merge conflicts and add "Closes #ISSUE_NUMBER" to the PR description

@ILIYANGERMANOV ILIYANGERMANOV added the ready to merge 🚀 PR can be merged after the CI pass label Jul 11, 2024
@ahsanishaq721
Copy link
Contributor Author

Added close in PR description but not understanding the merging conflicts. Can you help? What i have to do

@ILIYANGERMANOV
Copy link
Collaborator

Screenshot_20240711_203957_Chrome

click "Resolve Conflicts" and make sure that both Google dependencies are removed

@ahsanishaq721
Copy link
Contributor Author

@ILIYANGERMANOV done with resolved conflicts.

@ILIYANGERMANOV
Copy link
Collaborator

@ILIYANGERMANOV done with resolved conflicts.

@ahsanishaq721 now you need to fix the PR checks. Detekt is failing. Use the guide from the description

@suyash01
Copy link
Contributor

@ahsanishaq721 run gradle task for detekt in local and fix the issues until build is successful. For windows you can run .\gradlew detekt in the root of your project folder.

@ahsanishaq721
Copy link
Contributor Author

Thank u @suyash01

@ahsanishaq721
Copy link
Contributor Author

@ILIYANGERMANOV I am facing gradle issues, can I do some fixes in gradle in same branch and should i make another PR?

@ahsanishaq721
Copy link
Contributor Author

issue
@ILIYANGERMANOV facing this issue while runing detekt command.
My task was to remove google sign in related code which i have done. This is related to existing code. can you help me with that?

@suyash01
Copy link
Contributor

issue
@ILIYANGERMANOV facing this issue while runing detekt command.
My task was to remove google sign in related code which i have done. This is related to existing code. can you help me with that?

There should be detekt lint error message above the build failed error message. It is not highlighted red.

@ahsanishaq721
Copy link
Contributor Author

@suyash01 what should I do now? now detekt will be passed?

@ILIYANGERMANOV
Copy link
Collaborator

@ahsanishaq721 read the CI Troubleshooting guide. Click "Details" on the failed detekt check and you'll see

Property 'style>OptionalWhenBraces' is deprecated. Same functionality is implemented in BracesOnWhenStatements.
/home/runner/work/ivy-wallet/ivy-wallet/app/src/main/java/com/ivy/wallet/RootActivity.kt:207:1: First line in a method block should not be empty [NoEmptyFirstLineInMethodBlock]
/home/runner/work/ivy-wallet/ivy-wallet/app/src/main/java/com/ivy/wallet/RootActivity.kt:29:1: Unused import [NoUnusedImports]
/home/runner/work/ivy-wallet/ivy-wallet/app/src/main/java/com/ivy/wallet/RootActivity.kt:30:1: Unused import [NoUnusedImports]
/home/runner/work/ivy-wallet/ivy-wallet/app/src/main/java/com/ivy/wallet/RootActivity.kt:55:1: Unused import [NoUnusedImports]
/home/runner/work/ivy-wallet/ivy-wallet/screen/onboarding/src/main/java/com/ivy/onboarding/viewmodel/OnboardingViewModel.kt:25:1: Unused import [NoUnusedImports]
/home/runner/work/ivy-wallet/ivy-wallet/screen/onboarding/src/main/java/com/ivy/onboarding/viewmodel/OnboardingViewModel.kt:43:1: Unused import [NoUnusedImports]
/home/runner/work/ivy-wallet/ivy-wallet/app/src/main/java/com/ivy/wallet/RootActivity.kt:71:26: Private property onGoogleSignInIdTokenResult is unused. [UnusedPrivateProperty]
/home/runner/work/ivy-wallet/ivy-wallet/screen/onboarding/src/main/java/com/ivy/onboarding/viewmodel/OnboardingViewModel.kt:49:17: Private property ivyContext is unused. [UnusedPrivateProperty]

@ILIYANGERMANOV
Copy link
Collaborator

There are unused imports and fields

@ahsanishaq721
Copy link
Contributor Author

Ok, but i didn't import or add anything new in code. I have just removed the google sign in related code, which was my task.

@ahsanishaq721
Copy link
Contributor Author

@ILIYANGERMANOV creating PR again

@ahsanishaq721
Copy link
Contributor Author

@ILIYANGERMANOV I have done previously mentioned tasks mentioned by detekt. Now it is showing a longer list. These are existing issues and not related to my task I think.

@ILIYANGERMANOV
Copy link
Collaborator

@ILIYANGERMANOV I have done previously mentioned tasks mentioned by detekt. Now it is showing a longer list. These are existing issues and not related to my task I think.

You can either:

  • fix them
  • use Suppress for the harder
  • or add a Detekt baseline

Read the Troubleshooting guide

@ahsanishaq721
Copy link
Contributor Author

i have run ./gradlew detektBaseline command but it failed the build process.

@ILIYANGERMANOV
Copy link
Collaborator

@ahsanishaq721 can you trying using @Suppress for the hard fixes and fix the easier one? It shouldn't be that hard. Last week, I fixed like 400 or so 😅

@ahsanishaq721
Copy link
Contributor Author

@ILIYANGERMANOV At last all checks have been passed. I was first time working on detekt so sorry for delay.

Copy link
Collaborator

@ILIYANGERMANOV ILIYANGERMANOV left a comment

Choose a reason for hiding this comment

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

Nice job fixing the CI 👏 Found some issues that we need to fix before merging

gradle/libs.versions.toml Outdated Show resolved Hide resolved
gradle/libs.versions.toml Outdated Show resolved Hide resolved
app/build.gradle.kts Show resolved Hide resolved
@ILIYANGERMANOV ILIYANGERMANOV removed the ready to merge 🚀 PR can be merged after the CI pass label Jul 12, 2024
@ILIYANGERMANOV ILIYANGERMANOV added the requested changes Changes are needed. Please, apply them label Jul 12, 2024
@ahsanishaq721
Copy link
Contributor Author

@ILIYANGERMANOV I have done changes which you have requested. Please check. The checks are in progress.

Copy link
Collaborator

@ILIYANGERMANOV ILIYANGERMANOV left a comment

Choose a reason for hiding this comment

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

LGTM!

@ahsanishaq721
Copy link
Contributor Author

Now, lint have failed. It happened for the first time. Why?

@ILIYANGERMANOV
Copy link
Collaborator

Now, lint have failed. It happened for the first time. Why?

Retrying it, seems like a GitHub Actions problem

@ahsanishaq721
Copy link
Contributor Author

Ok done. Nice to work with you. It was my first small contribution to any open source project.

@ILIYANGERMANOV ILIYANGERMANOV merged commit 24555e3 into Ivy-Apps:main Jul 12, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requested changes Changes are needed. Please, apply them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove code related to google sign in
4 participants