-
Notifications
You must be signed in to change notification settings - Fork 42
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -319,14 +322,49 @@ private String generateAssertionsEntryPointClassContent(final Set<ClassDescripti | |
? determineBestEntryPointsAssertionsClassPackage(classDescriptionSet) | ||
: entryPointClassPackage; | ||
entryPointAssertionsClassContent = replace(entryPointAssertionsClassContent, PACKAGE, classPackage); | ||
|
||
Set<String> imports; | ||
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that we need the set of imports later in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is also a check that makes sure that there is If we recompute the imports in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think |
||
for (ClassDescription description : new TreeSet<>(classDescriptionSet)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not using a simple There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. return a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Will return a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
|
@@ -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) | ||
|
@@ -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)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ...) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't understood that last sentence. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method here returns a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about making the |
||
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())) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is better to use |
||
classToAssert = classDescription.getClassNameWithOuterClass(); | ||
} | ||
assertionEntryPointMethodContent = replace(assertionEntryPointMethodContent, CLASS_TO_ASSERT, | ||
classDescription.getFullyQualifiedClassName()); | ||
classToAssert); | ||
allAssertThatsContentBuilder.append(lineSeparator).append(assertionEntryPointMethodContent); | ||
} | ||
return allAssertThatsContentBuilder.toString(); | ||
|
@@ -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; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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()}. | ||
* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
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. | ||
*/ | ||
|
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. | ||
*/ | ||
|
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. | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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()); | ||
|
@@ -228,6 +236,11 @@ public void testGetSimpleNameWithOuterClass_notNestedClass() throws Exception { | |
assertThat(ClassUtil.getSimpleNameWithOuterClass(String.class)).isEqualTo("String"); | ||
} | ||
|
||
@Test | ||
public void testGetSimpleNameOuterClass_notNestedClass() throws Exception { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Note: starting test method is redundant since we already have an |
||
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"); | ||
|
There was a problem hiding this comment.
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
?There was a problem hiding this comment.
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