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
Adding support for json output #191
base: main
Are you sure you want to change the base?
Adding support for json output #191
Changes from 7 commits
f07558c
380f6d3
1dc75b6
b32b254
2badfed
c61f2d6
dba51d0
e783678
ce6e4ef
3cf5f6c
cd1e41a
b208617
9aa4a75
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.
Can we just use the underlying
Release
here? Might require aJsonConverter<PackageVersion>
that returns itsToString()
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.
Happy to put full release there, but given everything is currently
public
, everything would get includedThat would currently look like
Is there anything that would to be removed/hidden/etc
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.
Sorry, should have been clearer here. Can we just use more of the existing model objects/classes in the JSON above?
Packages
becomes an array ofResult
Release
(of whichResult
has 2,Installed
andLatest
) contains aPackageVersion
PackageVersion
use aJsonConverter<PackageVersion>
that returns just theToString()
interpretation of the version? (I may not have a proper understanding ofJsonConverter<T>
)So the JSON from above would look like:
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.
Let's add some tests for this bad boy as well
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.
Could we also name it
DoubleConverter
to match the convention of the base class andDateTimeConverter
?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.
Can we invert these ternaries (
... is not null
) to keep them in line with the others?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.
(this one might be moot now if the whole class disappears)