Skip to content

Use imports instead of FQN #97 #99

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,12 @@
import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.util.Collection;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import java.util.SortedSet;
import java.util.TreeSet;
Expand Down Expand Up @@ -319,14 +322,49 @@ private String generateAssertionsEntryPointClassContent(final Set<ClassDescripti
? determineBestEntryPointsAssertionsClassPackage(classDescriptionSet)
: entryPointClassPackage;
entryPointAssertionsClassContent = replace(entryPointAssertionsClassContent, PACKAGE, classPackage);

Set<String> imports;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extract imports resolution in a method, maybe resolveImports ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (entryPointAssertionsClassContent.contains(IMPORTS)) {
imports = extractImports(classDescriptionSet);
entryPointAssertionsClassContent = replace(entryPointAssertionsClassContent, IMPORTS, listNeededImports(imports));
} else {
imports = new HashSet<>();
}
String allEntryPointsAssertionContent = generateAssertionEntryPointMethodsFor(classDescriptionSet,
entryPointAssertionMethodTemplate);
entryPointAssertionMethodTemplate,
imports);
entryPointAssertionsClassContent = replace(entryPointAssertionsClassContent, ALL_ASSERTIONS_ENTRY_POINTS,
allEntryPointsAssertionContent);
return entryPointAssertionsClassContent;
}

private static String listNeededImports(Collection<String> imports) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@joel-costigliola joel-costigliola May 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@joel-costigliola joel-costigliola May 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

StringBuilder builder = new StringBuilder();
for (String anImport : imports) {
builder.append(format(IMPORT_LINE, anImport, LINE_SEPARATOR));
}
return builder.toString();
}

private static Set<String> extractImports(Set<ClassDescription> classDescriptionSet) {
Set<String> imports = new TreeSet<>();
Map<String, String> importedQualifiedName = new HashMap<>(classDescriptionSet.size() * 2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think importByClassName would be more expressive than importedQualifiedName

for (ClassDescription description : new TreeSet<>(classDescriptionSet)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@joel-costigliola joel-costigliola May 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😸 )

String outerClassName = description.getOuterClassName();
if (!importedQualifiedName.containsKey(outerClassName)) {
String toImport = description.getPackageName() + "." + outerClassName;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

imports.add(toImport);
importedQualifiedName.put(outerClassName, toImport);
}

if (!importedQualifiedName.containsKey(simpleAssertClassName(description))) {
String toImport = fullyQualifiedAssertClassName(description);
imports.add(toImport);
importedQualifiedName.put(simpleAssertClassName(description), toImport);
}
}
return imports;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it is, we can actually do this.

}

/**
* create the assertions entry point file, located in its package directory starting from targetBaseDirectory.
* <p>
Expand All @@ -352,7 +390,8 @@ private File createAssertionsFileFor(final Set<ClassDescription> classDescriptio
}

private String generateAssertionEntryPointMethodsFor(final Set<ClassDescription> classDescriptionSet,
Template assertionEntryPointMethodTemplate) {
Template assertionEntryPointMethodTemplate,
Set<String> imports) {
// sort ClassDescription according to their class name.
SortedSet<ClassDescription> sortedClassDescriptionSet = new TreeSet<ClassDescription>(classDescriptionSet);
// generate for each classDescription the entry point method, e.g. assertThat(MyClass) or then(MyClass)
Expand All @@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't understood that last sentence.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

customAssertionClass = simpleAssertClassName(classDescription);
}
assertionEntryPointMethodContent = replace(assertionEntryPointMethodContent, CUSTOM_ASSERTION_CLASS,
fullyQualifiedAssertClassName(classDescription));
customAssertionClass);
// 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())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

classToAssert = classDescription.getClassNameWithOuterClass();
}
assertionEntryPointMethodContent = replace(assertionEntryPointMethodContent, CLASS_TO_ASSERT,
classDescription.getFullyQualifiedClassName());
classToAssert);
allAssertThatsContentBuilder.append(lineSeparator).append(assertionEntryPointMethodContent);
}
return allAssertThatsContentBuilder.toString();
Expand Down Expand Up @@ -405,7 +452,11 @@ private static String abstractAssertClassNameOf(TypeName type) {
}

private static String fullyQualifiedAssertClassName(ClassDescription classDescription) {
return classDescription.getPackageName() + "." + classDescription.getClassNameWithOuterClassNotSeparatedByDots()
return classDescription.getPackageName() + "." + simpleAssertClassName(classDescription);
}

private static String simpleAssertClassName(ClassDescription classDescription) {
return classDescription.getClassNameWithOuterClassNotSeparatedByDots()
+ ASSERT_CLASS_SUFFIX;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,11 @@ public String getClassNameWithOuterClass() {
public String getClassNameWithOuterClassNotSeparatedByDots() {
return classTypeName.getSimpleNameWithOuterClassNotSeparatedByDots();
}


public String getOuterClassName() {
return classTypeName.getOuterClassName();
}

public String getPackageName() {
return classTypeName.getPackageName();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ public class TypeName implements Comparable<TypeName> {
private String typeSimpleNameWithOuterClass;
private String typeSimpleNameWithOuterClassNotSeparatedByDots;
private String packageName;
private String outerClassName;

public TypeName(String typeSimpleName, String packageName) {
if (typeSimpleName == null) throw new IllegalArgumentException("type simple name should not be null");
Expand Down Expand Up @@ -82,6 +83,7 @@ public TypeName(Class<?> clazz) {
this.typeSimpleNameWithOuterClass = ClassUtil.getSimpleNameWithOuterClass(clazz);
this.typeSimpleNameWithOuterClassNotSeparatedByDots = ClassUtil.getSimpleNameWithOuterClassNotSeparatedByDots(clazz);
this.packageName = clazz.getPackage() == null ? NO_PACKAGE : clazz.getPackage().getName();
this.outerClassName = ClassUtil.getSimpleNameOuterClass(clazz);
}

public String getSimpleName() {
Expand All @@ -104,6 +106,10 @@ private void setPackageName(String packageName) {
this.packageName = packageName == null ? NO_PACKAGE : packageName;
}

public String getOuterClassName() {
return outerClassName;
}

public boolean isPrimitive() {
return contains(PRIMITIVE_TYPES, typeSimpleName) && isEmpty(packageName);
}
Expand Down
18 changes: 18 additions & 0 deletions src/main/java/org/assertj/assertions/generator/util/ClassUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,24 @@ public static String getSimpleNameWithOuterClass(Class<?> clazz) {
return nestedClassName;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an example would be welcome

* @param clazz
* @return see description
*/
public static String getSimpleNameOuterClass(Class<?> clazz) {
if (isNotNestedClass(clazz)) {
return clazz.getSimpleName();
}
String outerClassName = null;
outerClassName = clazz.getName();
outerClassName = outerClassName.substring(clazz.getPackage().getName().length() + 1);
outerClassName = outerClassName.substring(0, outerClassName.indexOf('$'));
return outerClassName;
}

/**
* Gets the simple name of the class but, unlike {@link Class#getSimpleName()}, it includes the name of the outer
* class when <code>clazz</code> is an inner class, both class names are concatenated.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package ${package};

${imports}
/**
* A version of {@link BDDSoftAssertions} that uses try-with-resources statement to automatically call
* {@link BDDSoftAssertions#assertAll()} so that you don't forget to.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package ${package};

${imports}
/**
* A version of {@link SoftAssertions} that uses try-with-resources statement to automatically call
* {@link SoftAssertions#assertAll()} so that you don't forget to.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package ${package};

${imports}
/**
* Entry point for BDD assertions of different data types.
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package ${package};

${imports}
/**
* Entry point for BDD soft assertions of different data types.
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package ${package};

${imports}
/**
* Like {@link BDDSoftAssertions} but as a junit rule that takes care of calling
* {@link SoftAssertions#assertAll() assertAll()} at the end of each test.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package ${package};

${imports}
/**
* Like {@link SoftAssertions} but as a junit rule that takes care of calling
* {@link SoftAssertions#assertAll() assertAll()} at the end of each test.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package ${package};

${imports}
/**
* Entry point for soft assertions of different data types.
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package ${package};

${imports}
/**
* Entry point for assertions of different data types. Each method in this class is a static factory for the
* type-specific assertion objects.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,26 +22,31 @@
public interface NestedClassesTest {
@DataPoint
public static final NestedClass SNC = new NestedClass(OuterClass.StaticNestedPerson.class,
"OuterClass.StaticNestedPerson");
"OuterClass.StaticNestedPerson", "OuterClass");
@DataPoint
public static final NestedClass SNC_SNC = new NestedClass(OuterClass.StaticNestedPerson.SNP_StaticNestedPerson.class,
"OuterClass.StaticNestedPerson.SNP_StaticNestedPerson");
"OuterClass.StaticNestedPerson.SNP_StaticNestedPerson",
"OuterClass");
@DataPoint
public static final NestedClass SNC_IC = new NestedClass(OuterClass.StaticNestedPerson.SNP_InnerPerson.class,
"OuterClass.StaticNestedPerson.SNP_InnerPerson");
"OuterClass.StaticNestedPerson.SNP_InnerPerson",
"OuterClass");
@DataPoint
public static final NestedClass IC = new NestedClass(OuterClass.InnerPerson.class, "OuterClass.InnerPerson");
public static final NestedClass IC = new NestedClass(OuterClass.InnerPerson.class, "OuterClass.InnerPerson",
"OuterClass");
@DataPoint
public static final NestedClass IC_IC = new NestedClass(OuterClass.InnerPerson.IP_InnerPerson.class,
"OuterClass.InnerPerson.IP_InnerPerson");
"OuterClass.InnerPerson.IP_InnerPerson", "OuterClass");

public static class NestedClass {
private final Class<?> nestedClass;
private final String classNameWithOuterClass;
private final String outerClassName;

public NestedClass(Class<?> nestedClass, String classNameWithOuterClass) {
public NestedClass(Class<?> nestedClass, String classNameWithOuterClass, String outerClassName) {
this.nestedClass = nestedClass;
this.classNameWithOuterClass = classNameWithOuterClass;
this.outerClassName = outerClassName;
}

public String getClassNameWithOuterClass() {
Expand All @@ -55,5 +60,9 @@ public String getClassNameWithOuterClassNotSeparatedBytDots() {
public Class<?> getNestedClass() {
return nestedClass;
}

public String getOuterClassName() {
return outerClassName;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import static org.assertj.assertions.generator.util.ClassUtil.getClassesRelatedTo;
import static org.assertj.assertions.generator.util.ClassUtil.getNegativePredicateFor;
import static org.assertj.assertions.generator.util.ClassUtil.getPredicatePrefix;
import static org.assertj.assertions.generator.util.ClassUtil.getSimpleNameOuterClass;
import static org.assertj.assertions.generator.util.ClassUtil.getSimpleNameWithOuterClass;
import static org.assertj.assertions.generator.util.ClassUtil.getSimpleNameWithOuterClassNotSeparatedByDots;
import static org.assertj.assertions.generator.util.ClassUtil.getterMethodsOf;
Expand Down Expand Up @@ -211,6 +212,13 @@ public void should_return_inner_class_name_with_outer_class_name(NestedClass nes
assertThat(actualName).isEqualTo(nestedClass.getClassNameWithOuterClass());
}

@Theory
public void should_return_outer_class_name_for_nested_class(NestedClass nestedClass) {
String actualName = getSimpleNameOuterClass(nestedClass.getNestedClass());
assertThat(actualName).isEqualTo(nestedClass.getOuterClassName());
}


@Theory
public void should_return_inner_class_name_with_outer_class_name_not_separated_by_dots(NestedClass nestedClass) {
String actualName = getSimpleNameWithOuterClassNotSeparatedByDots(nestedClass.getNestedClass());
Expand All @@ -228,6 +236,11 @@ public void testGetSimpleNameWithOuterClass_notNestedClass() throws Exception {
assertThat(ClassUtil.getSimpleNameWithOuterClass(String.class)).isEqualTo("String");
}

@Test
public void testGetSimpleNameOuterClass_notNestedClass() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assertThat(ClassUtil.getSimpleNameOuterClass(String.class)).isEqualTo("String");
}

@Test
public void getClass_on_parameterized_List_should_return_List_class() throws Exception {
Method method = Generic.class.getMethod("getListOfInteger");
Expand Down
Loading