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

Refactoring the Usage of Deprecated Java APIs #11674

Closed
viktorkrimstein opened this issue Aug 26, 2024 · 16 comments · Fixed by #12056
Closed

Refactoring the Usage of Deprecated Java APIs #11674

viktorkrimstein opened this issue Aug 26, 2024 · 16 comments · Fixed by #12056
Labels
type: code-quality Issues related to code or architecture decisions

Comments

@viktorkrimstein
Copy link

Background

When running Code -> Analyze Code -> Run Inspection by Name -> Deprecated API Usage (Java | Code Maturity in the IntelliJ IDEA, I find 142 warnings about the usage of deprecated Java APIs in the code base.

Solution

Refactor the deprecated API usages without changing the functionality and performance of the given code base.

Acceptance Criteria

When running the workflow despcibed in the Background section, no depracted API usages are documented in the main branch.
In a situation, when the usage of deprecated APIs is currently necessary and inevitable, a clear JavaDoc is provided why this is the case and marked for later refactoring.

@koppor koppor added the type: code-quality Issues related to code or architecture decisions label Aug 26, 2024
@koppor
Copy link
Member

koppor commented Aug 26, 2024

@viktorkrimstein Please show up at my office of University of Stuttgart - or at my second office Sindelfingen; and we can pair on this. -- If you feel brave enough, you can open pull requests for yourself.

@koppor
Copy link
Member

koppor commented Aug 26, 2024

@viktorkrimstein Some more ideas: Rules for refactoring could be added to OpenRewrite. Maybe, you can craft minimal examples and create issues at the OpenRewrite repository. With that, the whole Java ecosystem would benefit.

@viktorkrimstein
Copy link
Author

@koppor I've already forked the project and am currently working an that. On the way finding more and more Bugs (e.g. that the ACS publication parser simply runs into HTTP 403: Forbidden). Trying to fix as much as I manage on the way.

Showing up at the University of Stuttgart would be great again but impossible due to family and work (Graduated with an M.Sc. years ago 😄)

I'm using JabRef since then and wanted to find some hustle to stay sane in the corporate IT world.

@koppor
Copy link
Member

koppor commented Aug 27, 2024

@koppor I've already forked the project and am currently working an that. On the way finding more and more Bugs (e.g. that the ACS publication parser simply runs into HTTP 403: Forbidden). Trying to fix as much as I manage on the way.

Nice. We love smaller pull requests. Maybe https://gitbutler.com/ helps to craft these. -- It has all your changes integrated locally, but can manage multiple upstream PRs for the changes.

Regarding fetchers, you might find some help at https://devdocs.jabref.org/code-howtos/fetchers.html.

I'm using JabRef since then and wanted to find some hustle to stay sane in the corporate IT world.

It was a similar story here. Glad to hear about others experiencing similar things 😅

@leaf-soba
Copy link
Contributor

I want to join this issue since I love refactor and code quality

@koppor
Copy link
Member

koppor commented Sep 24, 2024

I want to join this issue since I love refactor and code quality

Sure. Please try to do small commits / pull requests. Maybe one for each found inspection - or split up into the automatically done things and manual adaptions.

Also check if openrewrite has a recipe for it: Example: #11824

@leaf-soba But please keep in mind that the changes should be covered by tests (to avoid regressions) and that tests itself should only be modifeid if the code itself is modified.

We all do this project in our free time and have limited time to improve the software as is. Thus, please take refactorings as a learning for improving the whole system.

@leaf-soba
Copy link
Contributor

I want to join this issue since I love refactor and code quality

Sure. Please try to do small commits / pull requests. Maybe one for each found inspection - or split up into the automatically done things and manual adaptions.

Also check if openrewrite has a recipe for it: Example: #11824

@leaf-soba But please keep in mind that the changes should be covered by tests (to avoid regressions) and that tests itself should only be modifeid if the code itself is modified.

We all do this project in our free time and have limited time to improve the software as is. Thus, please take refactorings as a learning for improving the whole system.

OK, so far almost all code I contributed to this project are tested, except some test code itself or some really hard to test code.

@calixtus
Copy link
Member

If you open a pr within this issue scope, please add a 'refs' to this issue in the pr description, so we can track it.

@koppor
Copy link
Member

koppor commented Oct 8, 2024

It's Hacktoberfest this month - a good chance to resume work on this!

@leaf-soba
Copy link
Contributor

I want to replace Deprecated org.jabref.logic.bibtex.InvalidFieldValueException to org.jabref.logic.integrity.IntegrityCheck, but I find it very difficult to refactor it, since the IntegrityCheck need a lot of input parameters such as BibDatabaseContext,FilePreferences,CitationKeyPatternPreferences, but some class with InvalidFieldValueException don't have them, do I need add all these parameters to any class with InvalidFieldValueException? @koppor

@leaf-soba
Copy link
Contributor

I saw you put a @deprecated on org.jabref.logic.preferences.JabRefCliPreferences#getInstance:

/**
     * @return Instance of JaRefPreferences
     * @deprecated Use {@link CliPreferences} instead
     */
    @Deprecated
    public static JabRefCliPreferences getInstance() {
        if (JabRefCliPreferences.singleton == null) {
            JabRefCliPreferences.singleton = new JabRefCliPreferences();
        }
        return JabRefCliPreferences.singleton;
    }

but the CliPreferences is an interface which can't return a singleton, could you tell me what should I do to remove this Deprecated method? @koppor

@calixtus
Copy link
Member

I remember that InvalidFieldValueException was one of the more complicated things to refactor, since we wanted to move away from exception that control program flow and only use them for really exceptional situations (see https://github.com/HugoMatilla/Effective-JAVA-Summary?tab=readme-ov-file#57-use-exceptions-only-for-exceptional-conditions). I believe refactoring that is not just about moving methods and variables around, but requires a lot of more thinking and changes to the logical behavior.

@calixtus
Copy link
Member

Preferences should not be a Singleton, but created in the Launcher and then be injected.

@calixtus
Copy link
Member

If you want to refactor, please work first on the easy ones, there are plenty in the code. You start with the complicated ones. As we have day jobs it's pretty hard to keep up otherwise.

@leaf-soba
Copy link
Contributor

If you want to refactor, please work first on the easy ones, there are plenty in the code. You start with the complicated ones. As we have day jobs it's pretty hard to keep up otherwise.

I’d like to close this issue, but the last part has been quite challenging due to the InvalidFieldValueException deprecation problem. Apologies if this has taken up more time than expected, and thank you for your patience.

@koppor
Copy link
Member

koppor commented Oct 22, 2024

Most issues are fixed. For the methods we want to list strike-through, we keep the annotation (and added some explanaining text)

The two small left overs are listed as follows and are not worth to keep this issue open

  • org.jabref.model.groups.GroupTreeNode#setGroup(org.jabref.model.groups.AbstractGroup)
  • org.jabref.logic.ai.util.ErrorMessage#text

@viktorkrimstein I would be interested what kept you from fixing this issue. We have plenty of other small issues labeld with good first issues, which can be used as possibility to work on in the free time. -- @leaf-soba Thank you for fixing things. Also thanks to OpenRewrite supporting mass refactoring.

@koppor koppor closed this as completed Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: code-quality Issues related to code or architecture decisions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@koppor @viktorkrimstein @calixtus @leaf-soba and others