ClassFileImporter.importClasspath() ignores .withImportOption

[johanneskraemer]

The user guide contains the following snippet:

new ClassFileImporter()
    .withImportOption(ImportOption.Predefined.DO_NOT_INCLUDE_JARS)
    .withImportOption(ImportOption.Predefined.DO_NOT_INCLUDE_TESTS)
    .importClasspath();

Unfortunately ClassFileImporter.importClasspath() seems to override the import options and includes test classes:

importClasspath(new ImportOptions().with(ImportOption.Predefined.DO_NOT_INCLUDE_ARCHIVES));

The following code works as expected:

new ClassFileImporter().importClasspath(new ImportOptions()
    .with(ImportOption.Predefined.DO_NOT_INCLUDE_JARS)
    .with(ImportOption.Predefined.DO_NOT_INCLUDE_TESTS)
);

[codecholeric]

Yes, you're right, that behavior is counter intuitive ☹️ And even worse that this is the example from the user guide!
I've never been completely happy with the implicity of importClasspath() anyway... (still not sure if it's the best default, or if there should be a default ignoring anything anyway)
But in this specific case we should fix it and not ignore custom import options if specified...

[rweisleder]

When implementing #328 I was curious if we could rework importClasspath.

First, it's the only method that takes ImportOptions as an argument. No other method in ClassFileImporter has an equivalent method. Why is importClasspath so special?
Second, importClasspath(ImportOptions) imports the entire classpath and importClasspath() imports only plain class files (no archives). Same method name, different behaviour.

I implemented something on rweisleder/ArchUnit@cae6840.
To replace importClasspath(ImportOptions) I added

public JavaClasses importEntireClasspath() {
    return importLocations(Locations.inClassPath());
}

To replace importClasspath() I added

public JavaClasses importAllClasses() {
    return withImportOption(ImportOption.Predefined.DO_NOT_INCLUDE_ARCHIVES).importEntireClasspath();
}

And I deprecated both importClasspath methods.

WDYT?

[codecholeric]

Sounds all good to me 👍 AFAIR I looked at Degraph back then and copied the default which was to ignore JARs (but I might remember wrong, it's a long time ago 😉), that's where the different behavior comes from. I agree that this was not a good idea, I should have favored consistency.

I see two ways forward, a) introduce the new method like you did, deprecated importClasspath(..), then in a later version forward importClasspath(..) to use importEntireClasspath(..) and deprecate importEntireClasspath(..) b) simply replace the behavior of importClasspath(..) with the release of 1.0 including a big disclaimer (because that upgrade is one where probably everybody will check for breaking changes).
My worries with a) is, that there is no compile break ever. So if you don't look for deprecated, then at some point it will silently pop up, then vanish and the behavior changes 🤔 I don't know if that is better than just breaking the behavior in one shot with a disclaimer... WDYT?

[rweisleder]

Option c) would be to add importEntireClasspath() and importAllClasses() as suggested and deprecate both importClasspath(..) methods. In the next release (or 1.0) we could remove the importClasspath(..) methods. This would be a hard break of course. But everyone would be forced to think about which method they really want.

I'm still not sure if importAllClasses() is the best method name. Both importClasspath and importAllClasses pretend that they import all classes from the entire classpath. But most of the time they only import the class files from the current project/module.

[codecholeric]

Hmm, honestly I like importClasspath(..) better than importEntireClasspath(..), in particular because "entire" suggests something that clashes with what you fixed here 😉

importer.withImportOption(DO_NOT_INCLUDE_TESTS).importEntireClasspath()

How do I not include tests and yet import the "entire" classpath? 😉
And yes, importAllClasses(..) has its problems, too.

If I would redesign from scratch there would be one method importClasspath() which would by default import the whole classpath, including archives, but respect ImportOptions in a consistent way. And the importClasspath(importOptions) method would simply not exist. That would feel consistent to me... So maybe we should strive to get there somehow, just the path is open to me...

[rweisleder]

If I would redesign from scratch there would be one method importClasspath() which would by default import the whole classpath, including archives, but respect ImportOptions in a consistent way. And the importClasspath(importOptions) method would simply not exist. That would feel consistent to me... So maybe we should strive to get there somehow, just the path is open to me...

I also think that only one method importClasspath() would be the best solution.

This reminds of JavaClass.isNestedClass() vs JavaClass.isInnerClass() 😉 We'll keep one method without changing the signature but we change the semantic.
In this case, we should go for option b) and a big bang and a big disclaimer in the release notes. (which hopefully everyone reads...) This is much less effort than introducing an intermediate method and deprecate the original one and add the desired method later on. There is always one person (sometimes it's me 😄) who skips some releases and doesn't read the release notes...

[codecholeric]

Yeah, I agree. Also you will likely notice, because either the test will suddenly take a lot longer and you'll wonder (or it doesn't matter 😉) or fail with new errors about classes that were formerly not imported. It would be different if we would import less, then a test could silently not test the right thing anymore. But with this change the test would just test more and possibly fail with useless violations, and at that point I would guess everyone going through the release notes for some breaking changes 😉