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

Add checkstyle #817

Merged
merged 23 commits into from
Mar 26, 2022
Merged

Add checkstyle #817

merged 23 commits into from
Mar 26, 2022

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Mar 18, 2022

This PR adds checkstyle to the project. The motivation for doing this, and doing it in a single PR is the following: there have been many PRs lately that should fix one thing but while they are at it they also fix the formatting, making it far more difficult to review. That's why from now on any misformatting will be reported as an error (only test tasks though, not compileJava tasks so that we don't overload Jitpack's servers with useless checkstyle checks).

  • In commits "...downloader.Request", "...DashMpdParser", "...DonationLinkHelper", "...JsonUtils" I refactored some other small things (explained in the commits' descriptions). I feel bad for this as they are somewhat unrelated changes, but it would probably have taken me more time to reformat than to refactor ;-)
  • In commits "...MediaFormat..." and "...NewPipe..." instead of reformatting some methods I converted them to Java 8 Streams API altogether for the same reason as above ;-)
  • The checkstyle.xml I used is the same as NewPipe minus some Javadoc-related stuff. There is no need for strict rules over comments here in the extractor, as sometimes javadocs are just needed to clarify a small thing and having empty/meaningless @param or @throws is useless.
  • I had to use checkstyle version 9.3 instead of 10.0 as the latest requires compile SDK 11
  • While doing this PR I also found other places where the streams API could be used, but I refrained from adding unrelated changes here and will open a PR after this is merged. Please remind me if I forget when merging (and a reminder for myself: the local branch is called fix-warnings).

After this PR is merged I would propose to BAN any reformatting unrelated to the modified code in extractor PRs, unless the PR contains only reformats. The ways to format things that are not constrained by checkstyle are pretty subjective (e.g. whether to put multiple .foo().bar().baz() on the same line or split them), so it does not make sense to keep reformatting them, and in any case it should be done in separate PRs.

I hope this PR will be merged soon otherwise... many conflicts to solve ;-)

@Stypox Stypox force-pushed the checkstyle branch 2 times, most recently from 494fec2 to 93812c4 Compare March 18, 2022 23:36
@litetex litetex self-requested a review March 25, 2022 19:04
Copy link
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

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

Good work! (;^_^)ッ☆( ゜u゜)

Took me some time to review this... but mostly LGTM, only some minor things:

PS: Also please rebase the PR

build.gradle Outdated Show resolved Hide resolved
*/
public TimeAgoParser(PatternsHolder patternsHolder) {
public TimeAgoParser(final PatternsHolder patternsHolder) {
Copy link
Member

Choose a reason for hiding this comment

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

Note: This class definetly needs a refactoring after we merged this.

@litetex
Copy link
Member

litetex commented Mar 26, 2022

Fixed above problems myself.

Note: This PR breaks backwards compatibility with NewPipe → new major/minor release; no patch release + PR at NewPipe required!

Stypox and others added 21 commits March 26, 2022 19:37
With respect to NewPipe's checkstyle.xml, checkstyle is disabled for javadoc comments. There is no need for strict rules over comments here in the extractor, as sometimes javadocs are just needed to clarify a small thing and having empty/meaningless @param or @throws is useless.
Also remove useless null check on ItagItem.getItag() as that function already throws an exception if there is no itag
Also add comment about the class being unused and replace the fixLink function with Utils.stringToUrl()
Also use Java 8 streams and extract duplicate code to getInstanceOf function
Note: not all issues were fixed because MediaFormat and ServiceList use a specific formatting that makes sense for them
``contentFilters`` and ``sortfilter`` are get inside the ``ListLinkHandler`` and not the ``ListLinkHandlerFactory``
 ``ListLinkHandlerFactory`` only passes these values through when ``fromQuery`` is called
@litetex
Copy link
Member

litetex commented Mar 26, 2022

All Mix-Playlist tests are failing:

Could not get playlistData
org.schabi.newpipe.extractor.exceptions.ExtractionException: Could not get playlistData
	at org.schabi.newpipe.extractor.services.youtube.extractors.YoutubeMixPlaylistExtractor.onFetchPage(YoutubeMixPlaylistExtractor.java:100)
	at org.schabi.newpipe.extractor.Extractor.fetchPage(Extractor.java:60)
	at org.schabi.newpipe.extractor.services.youtube.YoutubeMixPlaylistExtractorTest$ChannelMix.setUp(YoutubeMixPlaylistExtractorTest.java:356)
	...

Also happens on the dev-Branch → can be ignored

@TobiGr
Copy link
Member

TobiGr commented Mar 26, 2022

Do you want to merge this before #810? I would not suggest it.

@litetex
Copy link
Member

litetex commented Mar 26, 2022

@TobiGr
Yes

@litetex
Copy link
Member

litetex commented Mar 26, 2022

Current test-results (mock):

grafik

Reasons:

@litetex litetex merged commit bb49f7d into TeamNewPipe:dev Mar 26, 2022
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.

3 participants