Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
    Giuseppe Barbieri
    @elect86
    want to give classgraph a try?
    Gabriel Selzer
    @gselzer
    @elect86 that would not solve the problem we are dealing with there
    Curtis Rueden
    @ctrueden
    I do think it would be fun to write a Discoverer backed by classgraph, though.
    Gabriel Selzer
    @gselzer
    As an aside, if you would like to make a ClassgraphDiscoverer, please declare it as a Service in the module-info

    I do think it would be fun to write a Discoverer backed by classgraph, though.

    Right, yeah. I just meant that it would not fix the problem we were talking about yesterday

    Curtis Rueden
    @ctrueden
    public class ClassGraphDiscoverer implements Discoverer {
      @Override   
      public <T> List<T> discover(Class<T> type) {
        List<T> instances = new ArrayList<>();
        try (ScanResult scanResult = new ClassGraph().enableAllInfo().scan()) {
          ClassInfoList impls = scanResult.getSubclasses(type);
          for (ClassInfo impl : impls) {
            try {
              Class<?> c = impl.loadClass();
              if (!type.isAssignableFrom(c))) continue; // Not an implementation of the requested type.
              T instance = (T) c.newInstance();
              instances.add(instance);
            }
            catch (IllegalArgumentException exc) {
              // NB: Could not load the class.
              // TODO: Log the exception.
            }
            catch (ExceptionTypesThatNewInstanceThrows exc) {
              // NB: Could not instantiate the class.
              // TODO: Log the exception.
            }
          }
        }
        return instances;
      }
    }
    Note that the above is completely untested. @elect86 Let us know if there is a better approach to using classgraph for this. In particular, I'm concerned that every call to discover may rescan the classpath (‽).
    Another concern is that getSubclasses may return abstract layers, not only "leaf class" implementations. So probably more checking is needed here before trying to instantiate each class. Maybe also checking for a no-args constructor? I dunno.
    I feel like classgraph must offer an easier way to do this, a la the ServiceLoader API, but I wasn't seeing it in the docs.
    Giuseppe Barbieri
    @elect86
    Can you guide me? @gselzer can you link me an example declared as a Service in module-info?
    @ctrueden the simplest test ever to copy/paste?
    on a side note, Kotlin has nice TODO(reason: String) :D
    Curtis Rueden
    @ctrueden

    @elect86 wrote:

    Can you guide me?

    I don't know classgraph, I thought you did. That's why I asked for your opinion. The question is: how do you get back one instance each of all implementing classes of a particular interface.

    Giuseppe Barbieri
    @elect86
    no, I meant if you can guide me into scijava
    Curtis Rueden
    @ctrueden
    In this case there's nothing to guide—it's a pure classgraph question.
    Giuseppe Barbieri
    @elect86
    ok, I can provide a PR, any info about tests?
    Curtis Rueden
    @ctrueden
    I'm not asking for a PR. Just whether you think the code I put above is a good way to use classgraph to solve that problem. Or if there is some API I missed.
    Giuseppe Barbieri
    @elect86
    it looks fine
    Curtis Rueden
    @ctrueden
    It's how you would do it?
    Giuseppe Barbieri
    @elect86
    you can tune the scan, but are we talking about an Annotation or?
    Curtis Rueden
    @ctrueden
    For now let's say no annotation.
    Will the try-with-resources result in a new classpath scan per discover call? Or does classgraph cache things?
    Giuseppe Barbieri
    @elect86
    as far as I recall, the scan is the bottleneck, so the less, the better
    ideally, you'd do this just once at the begin and then re-use it whenever you want to extract anything
    Curtis Rueden
    @ctrueden
    Agreed, but I wasn't sure if it was OK to save the ScanResult as a field of the Discoverer. That goes against the classgraph docs which say you should dispose of your ScanResult ASAP.
    Giuseppe Barbieri
    @elect86
    indeed, there seems to be some allocated resources alongside..
    Gabriel Selzer
    @gselzer
    @ctrueden I think this looks great.

    ideally, you'd do this just once at the begin and then re-use it whenever you want to extract anything

    I think at the end of the day you are going to be caching the results of the discover call, so I can't see a situation where you're going to be calling discover enough times to be worried about this?

    Curtis Rueden
    @ctrueden
    I'm worried about calling discover(Something.class) and then discover(SomethingElse.class) and then discover(AThirdThing.class) causing three full classpath scans.
    Gabriel Selzer
    @gselzer
    We could write some fancy logic to time it out after a couple seconds?
    Curtis Rueden
    @ctrueden
    Yeah, I was thinking something similar... that would be quite evil! Let's do it later, if/when we cross that bridge.
    Giuseppe Barbieri
    @elect86
    how often do you need to retrieve stuff?
    Gabriel Selzer
    @gselzer
    Idk, I think your intuition to follow the documentation and discard right away is probably correct
    In the short term
    Curtis Rueden
    @ctrueden
    It's not really about "how often" as much as "how many different types of things"
    Giuseppe Barbieri
    @elect86
    I imagine scijava just loads all the @ScijavaPlugins at the start and that's it
    Curtis Rueden
    @ctrueden
    And I'd say... maybe 20-50 types of things?
    For sure, for SciJava 2, loading everything with @Plugin would be straightforward.
    Giuseppe Barbieri
    @elect86
    also, there are some news starting from jdk16+..
    Gabriel Selzer
    @gselzer
    Alright, @ctrueden, I've been thinking more about how the TherapiOpInfoDiscoverer
    47 replies
    Curtis Rueden
    @ctrueden

    @gselzer Made some progress on the PR chain:

    I also rebased a couple of old topic branches and pushed them to stale/ prefix to clarify that they are old and may be obsolete now. I didn't finish going through all topic branches on the incubator yet, but the plan is to do that.

    Gabriel Selzer
    @gselzer
    Fantastic, many thanks @ctrueden. Did any of the merged PRs need fixes?
    2 replies
    Gabriel Selzer
    @gselzer

    @ctrueden thinking about giving the Discoverer a type variable:

    public interface Discoverer<T> {
      <U extends T> List<U> discover(Class<U> c);
    }

    Any opinions?

    Curtis Rueden
    @ctrueden
    @gselzer How does it help?
    You'll end up with Discoverer<?> e.g. from the ServiceLoader.
    Gabriel Selzer
    @gselzer
    Yeah, that's kind of what I'm running up against...
    I think it would make writing Discoverer implementations much cleaner
    But in practice it would be horrible to use...
    Curtis Rueden
    @ctrueden
    It would just avoid the need for a line like:
    if (!OpInfo.class.isAssignableFrom(c)) throw new IllegalArgumentException("Cannot discover classes of type " + c.getName());
    Gabriel Selzer
    @gselzer
    Well, I think we'd want a silent failure, but yes
    Curtis Rueden
    @ctrueden
    OK, so return empty list. Sure.