Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
    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.
    But either way you need to do the check, right?
    I think it's cleaner not to have the generic. You can ask any discoverer for any type, but some types they just don't care about.
    In which situations would having the type parameter make implementing the Discoverer easier?
    Gabriel Selzer
    @gselzer
    It would be rather nice for implementations of the TherapiDiscoverer implementations
    But I might just have to spend more time cleaning that up...

    To summarize, TherapiDiscoverer is an abstract class, whose subclasses need to implement (among less important things):

    protected abstract <U> U convert(Discovery<AnnotatedElement> e, Class<U> c);

    This method is called by

        @Override
        public <U> List<U> discover(Class<U> c) {
            if (!canDiscover(c))
                return Collections.emptyList();
            List<Discovery<AnnotatedElement>> elements = elementsTaggedWith(tagType());
            return convertEach(elements, c);
        }
    (I've found great use for Discovery in this one instance, that class will probably be moved into SciJava Discovery Therapi
    Curtis Rueden
    @ctrueden
    Why not put the <T> on the TherapiDiscoverer base class, then?
    Gabriel Selzer
    @gselzer
    I don't think that would help there. We want to say that the U is a T
    I'm just going to leave it the way it is
    Curtis Rueden
    @ctrueden
    Sounds good.
    Giuseppe Barbieri
    @elect86
    today I saw this and immediately though at when you, Curtis, told me that the community is kind of hating Java. If you got something light to do, you can hear it in background
    Gabriel Selzer
    @gselzer
    @ctrueden so I think that I am nearly to a state that builds in the discoverer refactoring
    The remaining question is how we should give Ops/OpCollections to the StaticDiscoverer in tests
    We currently give them classes, but the Discoverers are supposed to give a list of instances
    Gabriel Selzer
    @gselzer
    @ctrueden my goal today is to try and purge SciJava Common, finally
    Starting with SciJava Types, we have this NilConverter