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

Checkstyle assign on same line #8601

Merged
merged 4 commits into from
Jul 18, 2022

Conversation

litetex
Copy link
Member

@litetex litetex commented Jul 10, 2022

What is it?

  • Codebase improvement (dev facing)

Description of the changes in your PR

See #8170 (comment).

After some discussion with @Stypox a while ago (can't find the issue/PR anymore :/ ) where to put the = on a new line or not we agreed to put it not on the next line.

This PR was create to enforce this behavior.

This PR also removes some unused code that was found on the fly.

After the PR is merged the changes should also be applied to https://github.com/TeamNewPipe/NewPipeExtractor

Due diligence

@triallax triallax added the codequality Improvements to the codebase to improve the code quality label Jul 10, 2022
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

LGTM, except for the unused code removal. Thanks ;-)

@@ -18,159 +18,13 @@

package org.schabi.newpipe.util.urlfinder;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you can just remove things from this file at random. This file was taken from AOSP and modified, as the licence explains (though a comment would be better), so any change should be documented I guess.

Copy link
Member Author

Choose a reason for hiding this comment

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

The changes should already be documented by git (git blame, commit, etc).

Therefore I don't think further action is required as the first line /* THIS FILE WAS MODIFIED, CHANGES ARE DOCUMENTED. */ already describes this.

I added an

    //!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
    // CHANGED: Removed unused code //
    //!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

block so that we are completely on the safe side.

@litetex litetex force-pushed the checkstyle-assign-on-same-line branch from 02f836c to fc7fe67 Compare July 15, 2022 17:54
@litetex litetex force-pushed the checkstyle-assign-on-same-line branch from fc7fe67 to 3ba04f1 Compare July 15, 2022 18:00
@sonarcloud
Copy link

sonarcloud bot commented Jul 15, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Thanks :-)

@Stypox Stypox merged commit d9ff114 into TeamNewPipe:dev Jul 18, 2022
@litetex litetex deleted the checkstyle-assign-on-same-line branch July 24, 2022 12:48
@Stypox Stypox mentioned this pull request Aug 27, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codequality Improvements to the codebase to improve the code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants