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

Reducing redundant code #59

Closed
wants to merge 1 commit into from
Closed

Reducing redundant code #59

wants to merge 1 commit into from

Conversation

dhruvil-shah
Copy link
Contributor

In DepCleanMojo.java file while printing dependencies on console initial code was repeating same amount of lines so just added a method for it.
Also want to know if we could remove sizeOfDependencies from the parameters as it is globally accessible by all the methods.
Thanks and Regards
Dhruvil Shah

@ABHAY0O7
Copy link
Contributor

Hey! @dhruvil-shah,
All LGTM but I think you didn't follow the checkstyle ruleset.
Run mvn checkstyle:checkstyle
Open /target/site/checkstyle.html in any browser
Solve all the errors for your code and then try again.

Thank you
Abhay

@cesarsotovalero
Copy link
Collaborator

Hi @dhruvil-shah, thanks for the refactoring suggestion. Please do as @ABHAY0O7 says, just follow the Google Java style conventions. Your PR is good.

@cesarsotovalero cesarsotovalero self-assigned this Mar 25, 2021
@cesarsotovalero cesarsotovalero added the enhancement New enhacement or request label Mar 25, 2021
@dhruvil-shah
Copy link
Contributor Author

Hey @ABHAY0O7 Which ide you're using because I'm not able to configure checkstyle with VS Code

@ABHAY0O7
Copy link
Contributor

Hey! @dhruvil-shah, You didn't need to configure checkstyle it is already configured in the project's pom.xml file.
Just follow the steps and try to run these commands that I am mentioning here on your terminal.

  1. Run mvn clean
  2. Run mvn compile
  3. Run mvn site
  4. Run mvn checkstyle:checkstyle
    Note - If these above commands are not running successfully i.e. BUILD FAILED then you got some problems with the project setup.
  5. Now go to depclean-maven-plugin/target/site/checkstyle.html and open this file in any browser
  6. There you will found lots of errors but you have to solve only those errors which are coming from your code.
  7. After solving errors push the changes to the remote.

And in the end, I have a suggestion for you i.e. try to use IntelliJ IDEA ( I use this one ) or Eclipse IDE because these IDEs are specially designed for java based projects and related development.

Correct me @cesarsotovalero if I am wrong somewhere.
Thank you
Abhay.

@dhruvil-shah
Copy link
Contributor Author

Thanks much man @ABHAY0O7

@dhruvil-shah
Copy link
Contributor Author

Hey @ABHAY0O7
When I make PR without that files it fails build checks c69a6cb.
I saw your PR got accepted, so Is there any thing I'm missing.
Because on my local system the build's are successfull.

Thanks and Regards
Dhruvil Shah

@cesarsotovalero
Copy link
Collaborator

cesarsotovalero commented Mar 27, 2021

Correct me @cesarsotovalero if I am wrong somewhere.

You're totally right @ABHAY0O7. Thanks for your very concrete explanation of the setup. As you mentioned, it is very easy configuring Checkstyle in IntelliJ, there's even a very useful plugin for that that allows displaying the style errors in your working file.

@ABHAY0O7
Copy link
Contributor

Hey @ABHAY0O7
When I make PR without that files it fails build checks c69a6cb.
I saw your PR got accepted, so Is there any thing I'm missing.
Because on my local system the build's are successfull.

Thanks and Regards
Dhruvil Shah

Hey @dhruvil-shah,
The PR you have mentioned is already closed therefore I am not able to see where your build fails.
Or I will also give you advice that for one particular change you already created 3 PRs and this one is also still open.
So I will suggest you stay on one PR and try to commit to it not make different PRs for a single issue.
And close those PRs on which you are not working.

Thank you
Abhay

@ABHAY0O7
Copy link
Contributor

Hey @dhruvil-shah and @cesarsotovalero,
I want to know that, anyone from you is working on this PR?
If no then may I complete this?

Thank you

@codecov
Copy link

codecov bot commented Apr 4, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@e2929de). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 8dfc2d4 differs from pull request most recent head aba7cdd. Consider uploading reports for the commit aba7cdd to get more accurate results
Impacted file tree graph

@@           Coverage Diff            @@
##             master     #59   +/-   ##
========================================
  Coverage          ?   5.38%           
  Complexity        ?      20           
========================================
  Files             ?      26           
  Lines             ?     966           
  Branches          ?     133           
========================================
  Hits              ?      52           
  Misses            ?     902           
  Partials          ?      12           
Flag Coverage Δ Complexity Δ
unittests 5.38% <0.00%> (?) 0.00% <0.00%> (?%)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2929de...aba7cdd. Read the comment docs.

@cesarsotovalero
Copy link
Collaborator

cesarsotovalero commented Apr 4, 2021

Hi @dhruvil-shah, I just fixed the Checkstyle error and merged your changes, see #69. Looking forward to your next contribution.

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

Successfully merging this pull request may close these issues.

3 participants