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

Remove unused methods #846

Merged
merged 2 commits into from
Jun 19, 2022
Merged

Conversation

litetex
Copy link
Member

@litetex litetex commented May 9, 2022

After working on TeamNewPipe/NewPipe#7458 I noticed that these methods (and corresponding tests) are no longer required and can be removed.

This PR can be merged once TeamNewPipe/NewPipe#7458 is done

@litetex litetex merged commit f775155 into TeamNewPipe:dev Jun 19, 2022
@litetex litetex deleted the remove-unused-methods branch June 19, 2022 13:12
@AudricV
Copy link
Member

AudricV commented Jun 21, 2022

@litetex You should have checked with more caution because they are still used by NewPipe on the dev branch!

> Task :app:compileDebugKotlin
e: [...]/app/src/main/java/org/schabi/newpipe/error/ErrorInfo.kt: (68, 45): Unresolved reference: getNameOfService
e: [...]/app/src/main/java/org/schabi/newpipe/error/ErrorInfo.kt: (76, 45): Unresolved reference: getNameOfService
e: [...]/app/src/main/java/org/schabi/newpipe/error/ErrorInfo.kt: (98, 57): Unresolved reference: getNameOfService
e: [...]/app/src/main/java/org/schabi/newpipe/error/ErrorPanelHelper.kt: (109, 29): Unresolved reference: getNameOfService

> Task :app:compileDebugJavaWithJavac

[...]

[...]/app/src/main/java/org/schabi/newpipe/util/ServiceHelper.java:142: error: cannot find symbol
        final int serviceId = NewPipe.getIdOfService(serviceName);
                                     ^
  symbol:   method getIdOfService(String)
  location: class NewPipe

This PR should be reverted in my opinion.

@litetex
Copy link
Member Author

litetex commented Jun 21, 2022

@AudricV
Thank you for noticing, looks like I overlooked the Kotlin code.

A fix is provided with TeamNewPipe/NewPipe#8531

@TobiGr
Copy link
Member

TobiGr commented Jun 22, 2022

I am a little late on this. Can you please clarify why you removed the methods?
The extractor is not only used by NewPipe, but other applications, too. Removing methods without a prior warning / deprecation is not good practice.
I'd suggest to revert this PR as you already started to move one method into the app.

@litetex
Copy link
Member Author

litetex commented Jun 22, 2022

Can you please clarify why you removed the methods?

  1. I thought they were just used inside Moved subscription import/export options to (overflow) menu NewPipe#7458 but apparently I was wrong they were also used on some other places
  2. These methods are just shorthand methods for getService with an additional exception-check because somehow getService throws an checked Exception instead of simply returning null which would be a lot better.
  3. They are never used anywhere inside the extractor.
  4. grafik
  5. The tests are completely pointless
    a. The methods are in in my opinion way to simple to even have tests for
    b. If you write tests don't just write it for one testcase...

Removing methods without a prior warning / deprecation is not good practice.

I think people can handle removal of 2 barely used methods. Especially when the complete extractor architecture was/will be reworked with #810 and #862.

Also there was no prior warning when the behavior of returning null was changed to throwing a exception.

as you already started to move one method into the app.

I moved the method into the app, because a) it's used there and b) the new method doesn't have a use the unnecessary ExtractionException.

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.

4 participants