Use imports instead of FQN #97 #99

filiphr · 2017-05-21T17:36:58Z

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 · 2017-05-24T09:05:52Z

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.

joel-costigliola · 2017-05-24T08:33:07Z

src/main/java/org/assertj/assertions/generator/BaseAssertionGenerator.java

@@ -319,14 +322,49 @@ private String generateAssertionsEntryPointClassContent(final Set<ClassDescripti
        ? determineBestEntryPointsAssertionsClassPackage(classDescriptionSet)
        : entryPointClassPackage;
    entryPointAssertionsClassContent = replace(entryPointAssertionsClassContent, PACKAGE, classPackage);
-
+    Set<String> imports;


extract imports resolution in a method, maybe resolveImports ?

resolvedImports? As they are already resolved once they are in there. However, if we just use the map then we won't need this

joel-costigliola · 2017-05-24T08:35:06Z

src/main/java/org/assertj/assertions/generator/BaseAssertionGenerator.java

    entryPointAssertionsClassContent = replace(entryPointAssertionsClassContent, ALL_ASSERTIONS_ENTRY_POINTS,
                                               allEntryPointsAssertionContent);
    return entryPointAssertionsClassContent;
  }

+  private static String listNeededImports(Collection<String> imports) {


the name is confusing I was expecting it to return a list, rename to buildImports ?

Will do, just so you know, I used this name because it is already used to create the imports for the assertion classes. I thought there is some reason so I kept it

I was thinking to have the code looking like:

String imports = resolveImports(classDescriptionSet); entryPointAssertionsClassContent = replace(entryPointAssertionsClassContent, IMPORTS, listNeededImports(imports));

and do whatever we need to do in resolveImports to get all the imports.

I see that we need the set of imports later in generateAssertionEntryPointMethodsFor but I think it is better to recompute them in generateAssertionEntryPointMethodsFor as we are passing the Set<ClassDescription> which is enough to recompute the imports.
I like when we only pass the minimal parameters to a method, to generate assertThat methods we only need the template and the Set<ClassDescription>.

There is also a check that makes sure that there is ${imports} in the entry point template, if there is not then we don't do anything. Are you willing to enforce this and make this a breaking change?

If we recompute the imports in generateAssertionEntryPointMethodsFor then we need to pass a parameter that is going to tell whether we need to use imports or not.

Since it has been decided to go with the imports, I'm ok with the breaking change, users that have used their own templates would only have to add ${imports} at the top, it seems reasonable to me if we document that clearly.

joel-costigliola · 2017-05-24T08:37:57Z

src/main/java/org/assertj/assertions/generator/BaseAssertionGenerator.java

+  private static Set<String> extractImports(Set<ClassDescription> classDescriptionSet) {
+    Set<String> imports = new TreeSet<>();
+    Map<String, String> importedQualifiedName = new HashMap<>(classDescriptionSet.size() * 2);
+    for (ClassDescription description : new TreeSet<>(classDescriptionSet)) {


why not using a simple TreeSet instead of a map and just add everything ? the returned set will guarantee there will be no duplicates and the code will be really simple.

return a SortedSet to make it clear that imports are sorted.

If I add all the imports then we will have collisions when we have classes with the same name but different FQN. We might not need the Set if we just use the Map and do the check in the next method.

Will return a SortedSet to make it clear

in case of collision, we only add the import for the first class but not for the second, correct ?
this method needs comments to explain clearly hoe we deal with colliding class names.

Yes if we have a map then we would only add the FQN for the first simple class later one if an import exists we wouldn't do it.

can you add a comment with an example to make this clear for the next guy that will read the code ? (likely me in 6 months 😸 )

joel-costigliola · 2017-05-24T08:51:52Z

src/main/java/org/assertj/assertions/generator/BaseAssertionGenerator.java

@@ -362,12 +401,20 @@ private String generateAssertionEntryPointMethodsFor(final Set<ClassDescription>
      String assertionEntryPointMethodContent = assertionEntryPointMethodTemplate.getContent();
      // resolve class assert (ex: PlayerAssert)
      // in case of inner classes like Movie.PublicCategory, class assert will be MoviePublicCategoryAssert
+      String customAssertionClass = fullyQualifiedAssertClassName(classDescription);
+      if (imports.contains(customAssertionClass)) {


I think that if the import was not already known, we should add it to the import list but I feel that at this point we should already have it. WDYT ?

that makes me think that we should not pas the imports here, if we have done a good job at finding all of them, we can use simple class name everywhere (except for simple class name collision but does it apply here, not sure ...)

We need it so we can choose whether to use the simple class name or the FQN. Maybe better would be to pass a map from simple class name to FQN and if the simple class name matches and the FQN matches then we use that otherwise we use the FQN (Java 8 would be awesome here 😄).

The imports are created outside of this. I agree with you the best approach would be to compute the imports here (we already are iterating over the ClassDescription, but in order to do that we need to change the return so we can later add them to the entry point.

I haven't understood that last sentence.

This method here returns a String which is the content for entry points. If we want to compute the imports here (we can actually do this) then we need to return the content for the entry points and also the imports that need to be added to the entry point class

I was thinking to recompute the imports here only to be able to decide whether to use a FQN or not, replacing ${imports} would still be done in generateAssertionsEntryPointClassContent.
I prefer to have a simpler method signature (without Set<String> imports parameter) at the cost of recompute the set of imports which is not an overly expensive operation, I usually don't like to pass parameters when they can be inferred in the method itself.

What about making the generateAssertionEntryPointMethodsFor return some wrapper over the content and the imports? We'll need duplicate logic if we handle in in 2 places.

joel-costigliola · 2017-05-24T08:57:50Z

src/main/java/org/assertj/assertions/generator/util/ClassUtil.java

@@ -476,6 +476,24 @@ public static String getSimpleNameWithOuterClass(Class<?> clazz) {
  }

  /**
+   * Gets the simple name of the outer class, if the class is not nested then this is the same as
+   * {@link Class#getSimpleName()}.
+   *


an example would be welcome

filiphr · 2017-05-24T23:02:39Z

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 · 2017-05-25T06:38:46Z

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

joel-costigliola · 2017-05-25T07:50:55Z

src/main/java/org/assertj/assertions/generator/BaseAssertionGenerator.java

+        importedQualifiedName.put(simpleAssertClassName(description), toImport);
+      }
+    }
+    return imports;


is that imports = importedQualifiedName.values() ? if so we can get rid of the imports set

yes it is, we can actually do this.

joel-costigliola · 2017-05-25T07:58:40Z

src/main/java/org/assertj/assertions/generator/BaseAssertionGenerator.java

      // resolve class (ex: Player)
      // in case of inner classes like Movie.PublicCategory use class name with outer class i.e. Movie.PublicCategory.
+      String classToAssert = classDescription.getFullyQualifiedClassName();
+      if (imports.contains(classDescription.getPackageName() + "." + classDescription.getOuterClassName())) {


why not if (imports.contains(classToAssert) ? if there is a legitimate case for not using that we need a comment explaining why with an example.

So we can do imports only for first level classes, nested classes will go via the first level class.

For example for Movie.PublicCategory. We'll have an import for Movie and we then we can use Movie.PublicCategory in the template. What do you prefer more?

I think it is better to use Movie.PublicCategory over PublicCategory, inner classes name without their outer class can be ambiguous.

filiphr · 2017-05-28T09:18:07Z

@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

…ith the previous

filiphr · 2017-05-28T09:51:44Z

@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 · 2017-05-29T09:00:37Z

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 · 2017-05-30T19:46:18Z

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.

joel-costigliola · 2017-05-31T06:16:35Z

src/main/java/org/assertj/assertions/generator/BaseAssertionGenerator.java

+   */
+  private static SortedSet<String> extractImports(Set<ClassDescription> classDescriptionSet) {
+    Map<String, String> importedQualifiedName = new HashMap<>(classDescriptionSet.size() * 2);
+    for (final ClassDescription description : new TreeSet<>(classDescriptionSet)) {


do we need to create a new TreeSet<> to iterate on classDescriptionSet ?

Not necessarily, do we want to have some ordering of the imports, i.e. Do we want to first methods to always have a normal import and then the methods below to use FQN in case that is needed? What we can do is to pass a SortedSet<ClassDescription> to generateAssertionsEntryPointClassContent as we in any case create those down the line.

joel-costigliola · 2017-05-31T06:19:07Z

src/main/java/org/assertj/assertions/generator/BaseAssertionGenerator.java

+   * @return a sorted set with all the FQN that can be imported
+   */
+  private static SortedSet<String> extractImports(Set<ClassDescription> classDescriptionSet) {
+    Map<String, String> importedQualifiedName = new HashMap<>(classDescriptionSet.size() * 2);


I think importByClassName would be more expressive than importedQualifiedName

joel-costigliola · 2017-05-31T06:23:53Z

src/main/java/org/assertj/assertions/generator/BaseAssertionGenerator.java

+    for (final ClassDescription description : new TreeSet<>(classDescriptionSet)) {
+      final String outerClassName = description.getOuterClassName();
+      if (!importedQualifiedName.containsKey(outerClassName)) {
+        String toImport = description.getPackageName() + "." + outerClassName;


description.getPackageName() + "." + outerClassName should be extracted to a ClassDescription method (getOuterClassFullyQualifiedName ?), the same code is used later on in generateAssertionEntryPointMethodsFor.

joel-costigliola · 2017-05-31T06:25:34Z

src/main/java/org/assertj/assertions/generator/BaseAssertionGenerator.java

      // resolve class (ex: Player)
      // in case of inner classes like Movie.PublicCategory use class name with outer class i.e. Movie.PublicCategory.
+      String classToAssert = classDescription.getFullyQualifiedClassName();
+      if (resolvedImports.contains(classDescription.getPackageName() + "." + classDescription.getOuterClassName())) {


use the classDescription.getOuterClassFullyQualifiedName() (see previous comment)

joel-costigliola · 2017-05-31T06:30:24Z

src/test/java/org/assertj/assertions/generator/util/ClassUtilTest.java

@@ -229,6 +237,11 @@ public void testGetSimpleNameWithOuterClass_notNestedClass() throws Exception {
  }

  @Test
+  public void testGetSimpleNameOuterClass_notNestedClass() throws Exception {


rename the test using an english sentence to express what the test verifies then put _ instead of space.

Note: starting test method is redundant since we already have an @Test annotation

filiphr · 2017-06-01T20:27:29Z

@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 · 2017-06-01T20:35:49Z

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

joel-costigliola · 2017-06-01T20:36:43Z

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

filiphr · 2017-06-01T20:54:14Z

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 · 2017-06-11T07:04:32Z

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 · 2017-06-11T10:15:45Z

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 · 2017-06-11T21:23:23Z

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 · 2017-06-15T22:27:08Z

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 😄

Use imports instead of FQN assertj#97

6a992b1

joel-costigliola added this to the 3.0.0 milestone May 24, 2017

joel-costigliola reviewed May 24, 2017

View reviewed changes

joel-costigliola reviewed May 25, 2017

View reviewed changes

Address comments from the PR review. This commit should be squashed w…

8141039

…ith the previous

joel-costigliola reviewed May 31, 2017

View reviewed changes

joel-costigliola changed the base branch from master to main June 21, 2020 03:58

Use imports instead of FQN #97 #99

Are you sure you want to change the base?

Use imports instead of FQN #97 #99

Conversation

filiphr commented May 21, 2017

joel-costigliola commented May 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joel-costigliola May 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joel-costigliola May 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joel-costigliola May 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filiphr commented May 24, 2017

joel-costigliola commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filiphr commented May 28, 2017

filiphr commented May 28, 2017

joel-costigliola commented May 29, 2017

filiphr commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filiphr commented Jun 1, 2017

joel-costigliola commented Jun 1, 2017

joel-costigliola commented Jun 1, 2017

filiphr commented Jun 1, 2017

joel-costigliola commented Jun 11, 2017 • edited

filiphr commented Jun 11, 2017

joel-costigliola commented Jun 11, 2017

filiphr commented Jun 15, 2017

joel-costigliola commented May 24, 2017 •

edited

joel-costigliola May 25, 2017 •

edited

joel-costigliola May 27, 2017 •

edited

joel-costigliola May 27, 2017 •

edited

joel-costigliola commented Jun 11, 2017 •

edited