Manage when Loading versions... is displayed and alert the user to when no versions are found
#80
Conversation
Generate changelog in
|
| @@ -104,52 +113,82 @@ private void addLoadingElement(CompletionResultSet sortedResultSet) { | |||
| } | |||
|
|
|||
| private void handleDependencyWithoutWildcard( | |||
There was a problem hiding this comment.
We need to handle these as a chunk so that we can be sure that there are no versions for all repos rather than just no versions for one repo
| .collect(Collectors.toList()); | ||
|
|
||
| if (!pendingFutures.isEmpty()) { | ||
| CompletableFuture.anyOf(pendingFutures.toArray(new CompletableFuture[0])) |
There was a problem hiding this comment.
This should trigger a refresh when any one of the futures completes which should schedule less refreshes than just doing a refresh for each future I think
| addAndRefresh(key); | ||
| } | ||
|
|
||
| if (!versionCounts.isEmpty()) { |
There was a problem hiding this comment.
removes Loading Versions... from list of versions which is not strictly nessary as it sinks to the bottom but can be nicer when only a few versions are suggested
| addAndRefresh(key); | ||
| } | ||
|
|
||
| Integer packageCount = packageInRepo.stream() |
There was a problem hiding this comment.
We actually want to count the number of unique artifact names and not the size of packageInRepo
| @@ -47,6 +52,8 @@ public class VersionCompletionContributor extends CompletionContributor { | |||
| GroupPartOrPackageNameExplorer.getInstance(); | |||
| private final VersionExplorer versionExplorer = VersionExplorer.getInstance(); | |||
|
|
|||
| private final Queue<DependencyInfo> loadedDependencies = EvictingQueue.create(100); | |||
There was a problem hiding this comment.
Why do we need this extra state? It looks like it's used just for deciding whether to add the Loading versions... or not. Extra state is in general bad.
I think we can just work out whether to add loading versions/no results based on the info we get from VersionResults. Assuming we change the signature of VersionResults to:
public record VersionResults(Set<DependencyVersion> alreadyLoaded, Optional<CompletableFuture<Set<DependencyVersion>>> stillLoading) {}If in the combination of VersionResults:
- There is at least 1
stillLoading: addLoading versions... - There are 0
alreadyLoadedand 0stillLoading: addNo versions - Otherwise there are some
alreadyLoadedand 0stillLoading: add nothing, completion is complete.
There was a problem hiding this comment.
See update about the return type here: https://github.com/palantir/gradle-consistent-versions-idea-plugin/pull/80/files#r1842375371
There was a problem hiding this comment.
So I'm not sure if I'm thinking about this wrong but don't we need some state as we want to add Loading versions right at the top before any work is done so it pops up quickly. So we need a way at the start to determine if we should add Loading Versions - I guess its not essential but I think you would end up with a situation where the output said something like
Loading versions...
No versions found
| packageInRepo.stream().map(versionExplorer::getVersions).collect(Collectors.toSet()); | ||
|
|
||
| List<CompletableFuture<?>> pendingFutures = versionResults.stream() | ||
| .filter(results -> results.versions().isEmpty()) |
There was a problem hiding this comment.
This is actually super strange to me (I guess I'm still thinking in the mindset where you give a List<PackageInRepo> to VersionExplorer#getVersions).
For each getVersions call, is it the case that it's either some already loaded set of versions or it's a future to get more? If so, we should actually structure VersionsResults as an choice between the two somehow. I think you can do this really nicely in Java 21, but in Java 17 I think you can do something like:
sealed interface VersionsResult permits AlreadyLoadedVersions, StillLoadingVersions {}
record AlreadyLoadedVersions(Set<DependencyVersion>) {}
record StillLoadingVersions(CompletableFuture<Set<DependencyVersion>>) {}There was a problem hiding this comment.
I think there's definitely a type somewhere here that takes a set/list of VersionsResult and gives you methods like how many are still loading, how many versions are loaded etc
There was a problem hiding this comment.
So I have made quite a few changes, I've switched getVersions to take a List<PackageInRepo> and now it returns a new type VersionsResults which handles the results aggregation. This makes addToResults alot neater as a lot of the logic is handled in the results type
Before this PR
If not versions where found
loading versions...would remain present which could be very confusingAfter this PR
We now manage the state of
loading versions..., this is achieve by usingDependencyInfoas an indicator of if the line has been historically computed then display the message accordingly. This also allows us to display aNo versions foundmessage when we fail to find any versions==COMMIT_MSG==
Manage when
Loading versions...is displayed and alert the user to when no versions are found==COMMIT_MSG==
Possible downsides?
We now have to track which lines have been historically loaded, I opted for a
EvictionQueueso that it wouldn't get to big in memory but not sure this is the best method