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

prevent null as fix for WrongUsageOfMappersFactory #176

Conversation

hduelme
Copy link
Contributor

@hduelme hduelme commented Mar 10, 2024

I found a working fix for 2023.3. Sadly it doesn't work with older versions. Some things to note:

  • Still uses the internal IntelliJ api.
  • uses Reflection (could be avoided)
  • uses experimental features (asIntention())

@hduelme hduelme marked this pull request as draft March 10, 2024 16:44
@hduelme hduelme marked this pull request as ready for review March 10, 2024 18:46
@hduelme
Copy link
Contributor Author

hduelme commented Mar 10, 2024

See also #162

@hduelme
Copy link
Contributor Author

hduelme commented Mar 10, 2024

One way to fix it would be to be to just delete the Instance variable.

    private static class RemoveMappersFix extends LocalQuickFixOnPsiElement {

        private final String myText;
        private final String myFamilyName;

        private RemoveMappersFix(@NotNull PsiVariable element) {
            super( element );
            this.myFamilyName = MapStructBundle.message( "inspection.wrong.usage.mappers.factory" );
            this.myText = MapStructBundle.message( "inspection.wrong.usage.mappers.factory.remove.mappers.usage" );
        }

        @NotNull
        @Override
        public String getText() {
            return myText;
        }

        @Override
        public void invoke(@NotNull Project project, @NotNull PsiFile psiFile, @NotNull PsiElement psiElement, @NotNull PsiElement psiElement1) {
           psiElement.delete();
        }

        @Nls
        @NotNull
        @Override
        public String getFamilyName() {
            return myFamilyName;
        }
    }

This would drop the usage search

@filiphr
Copy link
Member

filiphr commented Mar 11, 2024

Thanks for your work on this @hduelme. I've been dabbling with this a bit as well. Perhaps we need to stop using the internal API from IntelliJ and just do the deletion on our side.

I didn't know that we could just do psiElement.delete();. Does #176 (comment) work on other versions as well? How does it behave if the instance field is used somewhere?

@hduelme
Copy link
Contributor Author

hduelme commented Mar 11, 2024

@filiphr the delete option would work on all version. The problem is that delete() does not check for usage.

@hduelme
Copy link
Contributor Author

hduelme commented Mar 11, 2024

The other solution would be to set the min supported version to 2023.3 and hope that the internal API does not change to much in the future.

@hduelme
Copy link
Contributor Author

hduelme commented Mar 11, 2024

I did a bit of testing and it seems that using the RemoveUnusedVariableFix also ignores if the variable is used or not and just deletes it. So no benfit from this anymore. What is RemoveUnusedVariableFix currently does is giving you a choose
grafik

But I think we don't want the extract option, so I removed it in my code and the result is that the variable is deleted ignoring if it is used or not.

@filiphr
Copy link
Member

filiphr commented Mar 11, 2024

Thanks a lot again for your work on this @hduelme. I think that we are converging on the solution in #179. Therefore, I'll close this one.

@filiphr filiphr closed this Mar 11, 2024
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.

2 participants