Use imports instead of FQN #97

[filiphr]

Fixes #97

This is a first go in trying to use imports instead of using FQN in the assertions class.

I am not sure if this is complete:

• Maybe do a check if the entry point is in the same location as the assert class and the domain object
• I was thinking that it might be good to add this as a configuration to the generator, so someone can turn it off until we are sure that it works ok. However, I saw that currently this is done on the caller side and not in the generator (for hiearchical for example)

[joel-costigliola]

first point is a nice to have but it does not hurt too much to have the import. second point yes unless it makes the code really more complicated.

I might have missed it but we should have a test with classes with the same simple name, ex: org.assertj.assertions.generator.data.nba.Player and org.assertj.assertions.generator.data.tennis.Player.

I have moved this to the 3.0 version to give use time testing this and because I'm going to release 2.1.0 soonish.

[filiphr]

Thanks for the feedback @joel-costigliola. I've replied to your comments inline. And here to your review comment:

first point is a nice to have but it does not hurt too much to have the import.

I'll see what we can do about this.

second point yes unless it makes the code really more complicated.

From what I saw one can pass the options to the BaseGenerator and then we can use them there. This will be much more simple then passing it via the API.

I might have missed it but we should have a test with classes with the same simple name, ex: org.assertj.assertions.generator.data.nba.Player and org.assertj.assertions.generator.data.tennis.Player.

Yes there is a test it is the AssertionsForClassesWithSameName.expected.txt and for this one there are imports for one Team and TeamAssert and FQN for the other.

I have moved this to the 3.0 version to give use time testing this and because I'm going to release 2.1.0 soonish.

Yes sure, 3.0 makes more sense so we can have t tested good. Looking forward to 2.1.0

[joel-costigliola]

I think we don't need the toggle feature / configuration flag, if we can make it work it is better to have imports.

[filiphr]

@joel-costigliola thanks for all the useful comments on the PR. I would also like to explain my reasoning behind some of the decisions. The reason why I want to consolidate the generation of the imports into one place is due to performance. If we need to compute the imports once in generateAssertionsEntryPointClassContent then we recompute them again in generateAssertionEntryPointMethodsFor (by reusing the same method so we get a set) and then we generate the entry points methods it means that we are going to iterate the classDescriptionSet at least 3 times and we are going to create a TreeSet of the classDescriptionSet 3 times:

• For the imports in generateAssertionsEntryPointClassContent
• For the imports in generateAssertionEntryPointMethodsFor
• To generate the actual entry point methods for

We can reduce this to 2 times, but then we will need to have the same logic we have in extractImports (will rename it) in the generateAssertionEntryPointMethodsFor. Do you think that this can have some performance hit in the execution. I am not sure the amount of classes that people have when using this. However, I am always for faster builds and with large classes that have assertions generated this might have an impact on it

[filiphr]

@joel-costigliola I update the PR with what we discussed in the comments.

While doing the last commit I thought about the templates. Are there some mandatory variables that a user has to use. For example: ${all_assertions_entry_points}, ${package} in the entry point classes template (there are others as well)? Will it make sense to create an issue where we would enforce the mandatory variables in the templates?

[joel-costigliola]

I never had performance issue with the generator, I don't think it is gonna be used on more than 10 000 classes, I favor simple code over more complex but more performant one, so let's avoid premature optimization, keep it as simple as possible and wait until we or someone else raise a performance issue.

As for mandatory variables, more or less everything is mandatory, should we verify the templates ? interesting question, I would say that users that change templates should test the generated result, it can't be our job, moreover even if the variables are there you can still screw up the generated code by using them at improper location - not much we can do about it.

[filiphr]

Sorry about premature optimization, I've been having problems with that lately (not with the generator), so I am in that mindset a bit :).

I addressed your previous comments. When you have time you can have another look at this. I think it is much cleaner now.

I created #107 for a discussion for the mandatory variables. It is just an idea to provide a fail fast alternative when a user does not use a variable.

[filiphr]

@joel-costigliola I just saw that #101 has been merged. This overhauls the type system that was used before. Can we say that it is safe to not use generics for this? The generator does not generate generic classes right?

[joel-costigliola]

what do you mean exactly by not use generics for this ?

[joel-costigliola]

Don't worry about the conflicts, I will fix them when integrating this PR

[filiphr]

I am wrong, the classToAssert can be a generic class. Taking this into consideration now we cannot generate the entry point without knowing whether the type is imported or not.

I am not worried about the conflicts the problems are more on a conceptual level.

If we have this entry point:

public com.example.MyGenericAssert assertThat(MyGeneric<com.example.Test> value) {
...
}

I even think that the code that will be created will be wrong. I think that we are going to generate something like MyGenericAssert<comExampleTest> (I have not tested it). There are no test that use generics.

Let's that we generate the entry points correctly, now we need to import MyGenericAssert, MyGeneric and Test. This means that we will need to know whether we are creating a class that is imported or not.

[joel-costigliola]

Yes that does not work out of the box in master but I think it is solvable. I'm experimenting and was able to generate this (forget about MyGeneric<Object> I haven't tackled it yet ):

public static org.assertj.assertions.generator.data.MyGenericAssert<T> assertThat(org.assertj.assertions.generator.data.MyGeneric<Object> actual) {",
   return new org.assertj.assertions.generator.data.MyGenericAssert<T>(actual);",
}

when giving:

public class MyGeneric<T> {

  public T field;

  public T getValue() {
    return null;
  }
}

I'm using Class.getTypeParameters to get the generic parameters information.

Still lot of work to do though ...

[filiphr]

Generics are quite tricky, especially on runtime when we have access only to the reflection API.

I can make the following suggestion, let's wrap up the generics first and then we go back to this one. What do you think?

As a direction for the generics I can propose the following: instead of doing everything in the single type we do it recursively, i.e. we have an Object that holds the raw type and the other generic parameters. We can have a factory that creates this types and this factory will know if a type can be imported (use non FQN) or not (use FQN). Then when we create the template we can just take the name for using from the type. Then for the imports we get what we can import from the type themselves.

I can point you to another project I am working on (MapStruct) and we have something like this. We generate Java classes with non FQN when possible.

[joel-costigliola]

Postponing this PR was my plan indeed (one thing at a time).

I'm refactoring the code to give ClassDescription the responsibility to hold all type information needed to generate assertions (raw type, generics parameters, computing Assert class names, etc ...). ClassDescription now has a TypeToken field to hold type information for the class it is describing (we could to extend that to generic parameters I guess).

Is that what you had in mind ?

I'm not sure I get why we would need the Factory you are talking about, I think ClassDescription could list the required imports and the BaseGenerator decide whether to use FQN or not, or maybe ClassDescription can decide that given a base package.

[filiphr]

Is that what you had in mind ?

Yes something like that. What I had in mind was ClassDescription to be responsible for outputting the type to String, the base generator would just invoke ClassDescription#asString() for example. The Factory class will make sure to create the ClassDescription correctly.

Let me know once you are done, so I can rebase this and continue working on it 😄