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

Improve Java expression injector both for better code completion and for more useable inspections, #89

Merged
merged 10 commits into from
Dec 27, 2021

Conversation

unshare
Copy link
Contributor

@unshare unshare commented Dec 19, 2021

primary enhancements being around generics.

@unshare unshare marked this pull request as ready for review December 19, 2021 22:15
@filiphr
Copy link
Member

filiphr commented Dec 25, 2021

Thanks for this PR @unshare, I think that I understand the problem you are trying to solve. Had a look at the code and it makes sense. Can you please have a look at the failing build and use the MapStruct code style guide?

In addition to that, can you have a look whether you can add a test case that will solve some of the problems that you've encountered. You can add the test in JavaExpressionInjectionTest

@unshare
Copy link
Contributor Author

unshare commented Dec 25, 2021

@filiphr
Code style: applied.
(Just a hint: one can save a code style scheme in the project and unignore .idea/codeStyles, so there would be no need for manual action for contributors.)

Build failure: can't reproduce, hence not sure.
@NotNulls were added by IDEA's quick fixes and they look correct. Never mind, please, re-run tests.

Tests: this is a tough one.

First of all, I'm not really familiar with IntelliJ code base and inner workings.
The only global side effect of current implementation incompleteness is a Local variable '__target__' is redundant warning. Moreover, it appears only if one runs Code->Inspect Code.... (More similar stuff when generics are in the vicinity.)
It may even be an IntelliJ bug.
There is no error anywhere in editor tab, hence nothing in getAllQuickFixes.
However, since I'm a real fan of language injections, I do use Edit <lang> Fragment quite often to offload escaping special characters off my head to the IDE.
When I saw what fragment is generated in my generics-laden case, I was compelled to write a better implementation.

As for the testing strategy, I'd rather find a way to emulate Edit Java Fragment and inspect the resulting fragment.
At the moment I have no clue how to do it and it will require a lot of effort with little benefit.

I do appreciate test-driven development, but at the moment the requirement for tests is too hard.
If you know the way to emulate Edit Java Fragment, please, do share.

@filiphr
Copy link
Member

filiphr commented Dec 26, 2021

Thanks for applying the code style.

Code style: applied.
(Just a hint: one can save a code style scheme in the project and unignore .idea/codeStyles, so there would be no need for manual action for contributors.)

I am aware about that. However, we would like to have the same code style here as in the MapStruct core repo. This means that if we need to change something it will need to be applied to multiple places. However, changing something might mean that other people won't pick it up as well. We need to see whether we should change this.

Tests: this is a tough one.

I mentioned this because I was not sure how you came up with the problem. However, after your explanation (using Edit Java Fragment), I understand how you did that. I personally did not know about that particular action. I will investigate that in more details. Thanks for sharing it with us.

@filiphr
Copy link
Member

filiphr commented Dec 26, 2021

I've added some tests for this @unshare and I also played around with the Edit Java Fragment.

I came up with something like:

import java.util.function.BiFunction;
import org.mapstruct.example.dto.CustomerDto;
import org.mapstruct.example.dto.Wrapper;
import org.mapstruct.example.mapper.CustomerMapper;

@SuppressWarnings("unused")
abstract class CustomerMapperImpl<T, U>
    implements CustomerMapper<T, U> {

    void __test__(
        CustomerDto customerDto,
        Wrapper<BiFunction<String, Number, CustomerDto>> wrapper
    ) {
        String __target__ = ;
    }
}

What do you think about this structure?

@unshare
Copy link
Contributor Author

unshare commented Dec 26, 2021

Aside from code style, it's essentially the same. Therefore, looks good to me.
There are definitely use cases not covered and there is always room for improvement.

I've added a commit that showcases how to use language injections to brighten up writing and handling test cases.
Every @Language-annotated string is subject to language injection and Edit <lang> Fragment goodness.
On the other hand, one has to use syntactically correct placeholders for substitution, so %s for String.format won't work.

(I do such stuff regularly, but mostly for SQL, HQL, JPQL, ...)

With regard to test cases, I see you didn't find the way to access generated fragments.
But you've expanded test coverage, especially with generics. Which is great!

@filiphr
Copy link
Member

filiphr commented Dec 26, 2021

I've added a commit that showcases how to use language injections to brighten up writing and handling test cases.

Yes I saw the commit. Really nice. Thanks for doing that.

With regard to test cases, I see you didn't find the way to access generated fragments.

I found the action that generates the test fragment. It is com.intellij.codeInsight.intention.impl.QuickEditAction. However, by looking at it I don't really see an easy way to access the file. Scratch that, I had another look and I think that it isn't that difficult to achieve this. I'll try to push it soon

@filiphr
Copy link
Member

filiphr commented Dec 26, 2021

I've managed to get hold of the generated "Java Fragment" in 3a196f8.

Good with the proper distinction, I was thinking about it as well

@filiphr
Copy link
Member

filiphr commented Dec 26, 2021

I am now quite happy with the improvements. Thanks a lot for the Edit Java Fragment pointer. It helped me improve the test cases a bit.

I've also added a test case for the interface vs class distinction.

I will try to merge this tomorrow and do a release with this improvement.

@filiphr filiphr merged commit 32da403 into mapstruct:master Dec 27, 2021
@filiphr filiphr added this to the 1.4.0 milestone Dec 28, 2021
@filiphr filiphr modified the milestones: 1.4.0, 1.3.1 Dec 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants