Add API for component groups #3286
Conversation
| The description of component groups provided by the package. Object fields can | ||
| be omitted if the corresponding list is empty. | ||
|
|
||
| ```typescript | ||
| interface ComponentGroups { | ||
| /** The list of component groups provided by the package. */ | ||
| newGroups: ComponentGroup[]; | ||
|
|
||
| /** The list of component groups that this package extends.*/ | ||
| extendedGroups: ExtendedComponentGroup[]; | ||
| } |
There was a problem hiding this comment.
If the fields can be ommitted, shouldn't they be marked with the ? ?
There was a problem hiding this comment.
Yes, you're right. I thought that ? indicates nullable fields in TypeScript, but turned out it also indicates an absence of a field.
| /** | ||
| * A string consisting of a namespace and a lirary name separated by the dot | ||
| * <namespace>.<library name>, i.e. `Standard.Base`. | ||
| */ | ||
| libraryName: string; | ||
|
|
||
| /** The module name. */ | ||
| moduleName: string; |
There was a problem hiding this comment.
May be worth clarifying if the moduleName contains the libraryName as prefix or if it is cut as it is implicit (probably the latter).
| The definition of a component group that extends an existing one. | ||
|
|
||
| ```typescript | ||
| interface ComponentGroup { |
There was a problem hiding this comment.
| interface ComponentGroup { | |
| interface ExtendedComponentGroup { |
| The component group provided by a library. | ||
|
|
||
| ```typescript | ||
| interface ComponentGroup { |
There was a problem hiding this comment.
| interface ComponentGroup { | |
| interface LibraryComponentGroup { |
Shouldn't the name here be consistent with the header?
| .collect { case Some(config) => | ||
| config | ||
| } |
There was a problem hiding this comment.
Again, shouldn't libraries for which the config could not be loaded be indicated somehow? Just ignoring them feels a bit bad. If not in this iteration, at least a TODO could be left for the future - otherwise I'd think it will be unlikely this gets revisited.
| * @param localLibraryManager reference to the local library manager actor | ||
| * @param publishedLibraryCache the cache of published libraries | ||
| */ | ||
| class LibraryGetPackageHandler( |
There was a problem hiding this comment.
The implementation of this class looks so similar to LibraryGetMetadataHandler that I'm wondering if maybe it would be worth to separate out the shared logic to avoid repeating it?
| ) | ||
| } | ||
|
|
||
| "skip component groups extending nothing" in { |
There was a problem hiding this comment.
Again, shouldn't this be reported somewhere as a warning?
If I make a typo in specifying what existing group I want to extend, my extensions will just not show up - not very helpful when trying to figure out what was the issue. Not saying this is necessary from the beginning, but at least an indication that it should be added at some point would IMO be good.
| client.send(json""" | ||
| { "jsonrpc": "2.0", | ||
| "method": "editions/listDefinedComponents", | ||
| "id": 0, | ||
| "params": { | ||
| "edition": { | ||
| "type": "NamedEdition", | ||
| "editionName": $currentEditionName | ||
| } | ||
| } | ||
| } | ||
| """) | ||
|
|
||
| client.expectJson(json""" | ||
| { "jsonrpc": "2.0", | ||
| "id": 0, | ||
| "result": { | ||
| "availableComponents" : [ ] | ||
| } | ||
| } |
There was a problem hiding this comment.
Maybe you could somehow fake the current edition to have any component groups? Feels like the test is a bit incomplete if only the path leading to 'no results' is tested.
|
|
||
| /** A [[PublishedLibraryProvider]] that just provides libraries which are | ||
| * already available in the cache. | ||
| */ | ||
| class CachedLibraryProvider(caches: List[ReadOnlyLibraryCache]) | ||
| extends PublishedLibraryProvider | ||
| with PublishedLibraryCache { | ||
| extends PublishedLibraryCache { |
There was a problem hiding this comment.
This comment is not valid anymore - this class no longer implements the PublishedLibraryProvider interface, like suggested in the doc comment.
There was a problem hiding this comment.
Overall - looks really great; it took me quite a while to review this as this is adds quite a lot of functionality.
- Some comments on documentation that I think should be addressed before merging (inconsistent names etc.)
Then some lower priority stuff but would be cool if addressed:
2. As written in comments above - in multiple places we swallow errors regarding parsing and handling of component groups. This seems bad to me - the users will be confused why their defined groups do not show up without getting any cues. While I understand this may not be the most pressing feature at this stage - I think it would be good to at least mark these relevant places with a TODO and adding a task to address this - so that we can keep track of the places that will need to be revisited to have proper error handling.
3. Some weird style where a mutable data structure is handling using operations associated with immutable structures (folding where the accumulator is a mutable map) - while not incorrect, it just looks strange (that is a bit subjective though). I'd suggest maybe changing this to be more uniform - either by using immutable structures or by writing more imperative code using mutable structures - the mixup just looks a bit weird.
4. If possible, would be cool to integration test the editions/listDefinedComponents endpoint - as (unless I missed something which is not out of question) seems like is only tested with the scenario where no components are returned - which seems a bit lacking.
|
@radeusgd thanks for the review. I managed to address most of your comments.
|
There was a problem hiding this comment.
Awesome!
- Implemented config verification and error logging. In the future can extend it and send the verification errors to the client
Looks perfect. I think we should add a way to send the errors to the client, but totally understandable to make this a separate task (and probably not the top-priority). Do you think it's worth creating a task in Pivotal to at least keep it somewhere in the Icebox?
- I changed the data structures returned from grouping combinators to immutable ones. Although I believe that folding with the mutable reference is fine, because it does not affect the referential transparency (you will still get the same result as with immutable one)
I totally agree that folding a mutable reference is fine - in the sense that it is correct. It is just an uncommon pattern which IMO may be confusing when reading it (mixing imperative and functional constructs like this). But that is just a subjective code-style opinion, so I'm not pushing it.
- I was not able to mock the edition in order to implement a proper test for the
listDefinedEditions. The result of this test will be filled with component groups when we add them to the standard library
Yeah, I expected this may be problematic. Would be good to have it, but if the component groups are going to be added to the standard library relatively soon, I guess we are good (not much value in implementing a mock which will not be necessary soon).
Would be cool to temporarily add a component group to stdlib and at least test it manually/locally - just as a sanity check. Unless you already did that :)
Pull Request Description
PR ads two new commands to the Language Server protocol
library/getPackageto get the package definition of the provided libraryeditions/listDefinedComponentsto get the list of available components in the provided editionImportant Notes
The
library/getPackagemethod contains an additionalrawfield requested by @farmaazon for debugging purposes. Therawfield contains the contents ofpackage.yamlas-is. This field should be removed once the integration is complete and the package syntax is stabilized.To get the available components, one can either:
editions/listDefinedComponentsmethod that returns a ready-to-use list of component groups defined by the libraries in the requested edition.library/listLocalandeditions/listDefinedLibraries), and then get the package configs using thelibrary/getPackagecommand.Checklist
Please include the following checklist in your PR:
./run distand./run watch.