Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
#131: Support for tool dependencies #145
#131: Support for tool dependencies #145
Changes from 11 commits
a9f80db
41d23ef
96f807f
ecd1df3
b222aa5
6080c80
f7ff2c2
e44d547
4a6c8e9
2805f0c
67c5c9c
cc8c0cb
ed0b0f3
18884c2
f2ed771
32e4bd1
48e3437
a3d82b9
21781f2
e4ad57b
b9846f8
52840dc
fbcbf31
0a9b2b5
8f04879
584c5f4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general it is better to keep such structures internally to simplify the JSON mapping with Jackson but for the public API instead provide rather something like
public List<DependencyInfo> getDependencies(VersionIdentifier toolVersion)
or if you want to keep logic out of the data objects and keep them dump and stupidpublic List<ToolVersionDependencyInfo> getDependencies()
. Looking at this method some new member in the team will not have a clue what the semantic of this map will be. What are the keys of typeString
? Truth is they areVersionIdentifiers
(or are they patterns or may they also be version ranges)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my Opinion it also makes sense to only change the String to VersionRange, and that's what I've done. Please let me know if this really makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets agree on this
Map<String, List<DependencyInfo>>
as it makes the JSON structure simpler.But we can omit the root object and you can directly deserialize to
Map
:https://www.baeldung.com/jackson-map#bd-3-mapltobjectobjectgt-deserialization
I would propose to keep some class like
DependencyJson
that contains the code to read the JSON and provides the result asDependencyJson
wrapping theMap
in Java without the root object being in the JSON syntax.Then
DependencyJson
should also implement validation logic so that we can render a warning if we hit something like this:So in other words valid JSON should not contain overlapping
VersionRange
s.