Skip to content
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

Add typed overloads for methods taking class names #549

Merged
merged 2 commits into from Aug 16, 2021

Conversation

larsgrefer
Copy link
Contributor

This allows a more typesafe way of working with scan results.

@lukehutch
Copy link
Member

@larsgrefer thank you for this contribution!

A couple of things --

(1) You added some Assert checks to these methods, to ensure the parameter types were reasonable. There was a request recently to drop these sorts of checks. What do you think about this?: #543

(2) Actually using Class<?> references was standard practice in the early days of ClassGraph (back when it was called FastClasspathScanner), and I changed the entire API to use class names rather than class references, because using a class reference triggers classloading and class initialization, and I had several users run into problems where e.g. a superclass' Class<?> reference was loaded in one classloader, and all the subclasses found by FastClasspathScanner were loaded by a different classloader. This would mean that the subclass couldn't be cast to its own superclass without throwing a ClassCastException. ClassGraph has evolved a lot since then though, and it goes out of its way to not load or initialize classes. Additionally it tries to load classes with its own resource-based classloader only as a very last resort, if no other classloader can load a given class. So I think the situation is better now, and maybe these methods can be added back to ClassGraph.

@larsgrefer
Copy link
Contributor Author

(1) I'd argue that the Assert statements are just proper input validation, but we could add some javadoc to the methods explaining that an illegal argument will trigger an IllegalArgumentException.

(2) I don't want to force any classloading nor change the way how classgraph works internally. This PR only offers a way to better deal with cases, where some known classes are already loaded anyway.

@lukehutch
Copy link
Member

Yes, I think this is all very reasonable. Thanks again for the great code contribution. I'll merge it and push out a release shortly.

@lukehutch lukehutch merged commit f35f183 into classgraph:latest Aug 16, 2021
@lukehutch
Copy link
Member

Released in 4.8.115.

lukehutch added a commit that referenced this pull request Aug 16, 2021
lukehutch added a commit that referenced this pull request Aug 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants