-
Notifications
You must be signed in to change notification settings - Fork 495
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
Expose export formats in native API #10739
Expose export formats in native API #10739
Conversation
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.
@julian-schneider good idea! Thanks for the pull request. I left you some comments.
Get Export Formats | ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
||
Get the available export formats. The response contains a list of objects each containing the display name and format name for an available exporter. |
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.
Does this include external exporters? I assume so. Can we please mention they are included 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.
As far as I know, all exporters, standard and custom, are managed by the ExportService
, and are consequently all listed. I improved the docs in 3b2c8e5.
Thanks for your feedback @pdurbin - I will be absent for two weeks, so some time will pass before I revise this. See you then! |
The response is now an object, and contains more properties per format, as suggested. New response looks like this: (expand response)
|
@pdurbin I believe I have addressed all the feedback you gave, have a nice day! |
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.
I haven't run the code yet (or in a while, I forget) but I left a couple more suggestions.
Please also merge the latest from develop.
@johannes-darms I do like the PR a lot. @cmbz @scolapasta what do you think? Can we put a 6.5 milestone on it or at least put it in "sprint ready"? Also, I agree with Johannes that the SPA will need this some day. We might want to see what @ekraffmiller @ChengShi-1 @GPortas and @g-saracca think about the output, if they are happy with it. |
2024/10/15: Added to sprint ready after conversation with @pdurbin |
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.
@julian-schneider this is looking really good. I left you a few comments and a PR-for-this-PR ( julian-schneider#1 ) to consider. Also, can you please merge the latest from "develop" into your branch? Thanks!
for (String[] labels : instance.getExportersLabels()) { | ||
try { | ||
Exporter exporter = instance.getExporter(labels[1]); |
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 lookup seems a bit awkward but it's not your fault. I wonder if we should add a method to ExportService like this...
public Collection<Exporter> getExporters() {
return exporterMap.values();
}
... so that it can be used like this in places like Info.java:
for (Exporter exporter : instance.getExporters()) {
//...
}
But I dunno. Maybe there's a reason why we don't offer a "getExporters" method like this.
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.
I agree that the steps I had to use to get the exporters were a bit awkward. If you think it would be in scope, I can add a method like that to this PR.
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.
No, let's think about it outside this PR. You're welcome to create an issue, though, if you like!
remove Gson and test based on content, not string match IQSS#10739
Co-authored-by: Philip Durbin <[email protected]>
@julian-schneider tests are failing in InfoIT.testGetExportFormats. Can you please take a look?
|
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.
As of f3b72c6 tests are passing: https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-10739/9/testReport/
Thanks, @julian-schneider! Approved.
In DMs we did discuss how if there's a lot of churn in our formats we can do some mocking. Still, it's valuable to actually exercise the API endpoint itself, even if we make the assertions less strict in the future.
No issues with PR discovered. |
What this PR does / why we need it: The PR adds a method that exposes the full list of available exporters (display- and format names). This is especially useful for instances that have added custom formats.
Which issue(s) this PR closes: I know no pre-existing issues for this.
Suggestions on how to test this: New method found here:
/api/info/exportFormats
Does this PR introduce a user interface change? If mockups are available, please link/include them here: No, just API and documentation changes.
API answer looks like this:
Rendered docs entry looks like this:
Live version: https://dataverse-guide--10739.org.readthedocs.build/en/10739/api/native-api.html#get-export-formats
Is there a release notes update needed for this change?:
New API method for listing the available exporters. Found at
/api/info/exportFormats
, produces display names and format names.