Consolidate ClassFileImporter.importClasspath(..) with other APIs - #958
Conversation
Stumbled over this outdated frustrated comment about the Java 7 type system and decided to clean that up real quick. Signed-off-by: Peter Gafert <[email protected]>
codecholeric
changed the title
ClassFileImporter.importClasspath(..) with other APIClassFileImporter.importClasspath(..) with other APIs
2618f69 to
baa845a
Compare
hankem
left a comment
hankem
left a comment
There was a problem hiding this comment.
The old version of new ClassFileImporter().importClasspath() was indeed strange. Thanks for making this consistent!
| [source,java,options="nowrap"] | ||
| ---- | ||
| JavaClasses classes = new ClassFileImporter().importClasspath(new ImportOptions()); | ||
| JavaClasses classes = new ClassFileImporter().importClasspath(); |
There was a problem hiding this comment.
L29 and L50, as well as 009_JUnit_Support.adoc's L107, still refer to
ImportOptions
which could IMO be changed to
ImportOptions
There was a problem hiding this comment.
Hmm, yes, I was pondering about it. Thought it also looks weird to have the 's' outside (and I think in other Javadocs, etc., I sometimes put the whole plural into the code as well), but maybe it's better to change it 🤔
There was a problem hiding this comment.
A JavaDoc link is different because it unambigiously targets the ImportOption class.
But in the documentation, you don't have a link. When I see "ImportOptions" there, I'd assume that there is a class (or some other code construct) ImportOptions and be confused (if not even misled when I find the package-private class ImportOptions). I'd use "ImportOptions" to separate which part is code (ImportOption) and which part is grammar ("-s").
This class does not provide any real value as public API versus simply using the standard interface `Collection<ImportOption>`. We will next release new major version `1.0.0`, thus we can remove it from the public API, so we won't have to maintain it in the future. Signed-off-by: Peter Gafert <[email protected]>
All other import methods behave consistently by taking into account all the `ImportOptions` that have been added via `withImportOption(..)` and then only controlling where to look for class files by the final `import...()` method. Only `importClasspath()` transparently adds an `ImportOption` to exclude archives from the import. This creates a surprising API, also that the only way to really import the whole classpath then is to use `importClasspath(emptySet())` with an empty set of `ImportOptions` passed to the `importClasspath(..)` method. We now consolidate these APIs to make `importClasspath()` behave exactly like the other methods. We remove the version `importClasspath(Collection<ImportOption> options)` as there is no advantage now over using `withImportOptions(options).importClasspath()` like with all other import methods. Right now is the best moment to do this breaking change, since we are about to release version `1.0.0`. Signed-off-by: Peter Gafert <[email protected]>
baa845a to
cba15f5
Compare
2 participants
All other import methods behave consistently by taking into account all the
ImportOptionsthat have been added viawithImportOption(..)and then only controlling where to look for class files by the finalimport...()method. OnlyimportClasspath()transparently adds anImportOptionto exclude archives from the import. This creates a surprising API, also that the only way to really import the whole classpath then is to useimportClasspath(emptySet())with an empty set ofImportOptionspassed to theimportClasspath(..)method.We now consolidate these APIs to make
importClasspath()behave exactly like the other methods. We remove the versionimportClasspath(Collection<ImportOption> options)as there is no advantage now over usingwithImportOptions(options).importClasspath()like with all other import methods. Right now is the best moment to do this breaking change, since we are about to release version1.0.0.