-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add use case to retrieve file counts by type, access and category #88
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.
Looks good! I appreciate the included refactor 👏 I'm going to start integrating this into the frontend right away
I added a comment about a naming but it's a matter of preference so I'm not blocking this PR, if you agree with the comment and want to do the change just go ahead and ask for a new approval afterwards
@@ -26,6 +26,18 @@ export abstract class ApiRepository { | |||
}); | |||
} | |||
|
|||
protected buildApiEndpoint(resourceName: string, operation: string, resourceId: number | string = undefined) { |
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.
love the refactor using this method 👍
public async getDatasetSummaryFieldNames(): Promise<string[]> { | ||
return this.doGet('/datasets/summaryFieldNames') | ||
return this.doGet(this.buildApiEndpoint(this.resourceName, 'summaryFieldNames')) |
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 think I like it better in FileRepository where the dataset resource name has this naming this.datasetsResourceName
return this.doGet(this.buildApiEndpoint(this.datasetsResourceName, 'summaryFieldNames'))
I think it helps visually seeing which resource is calling, specially because it seems like the resource called is not always the one in the repository name
@GPortas I think this PR wasn't published to the npm registry https://github.com/IQSS/dataverse-client-javascript/pkgs/npm/dataverse-client-javascript can you publish it? |
What this PR does / why we need it:
Adds use case for getting file counts, as requested in #83.
In addition, it includes a small refactoring for the API endpoint building in the API repositories.
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this:
Is there a release notes update needed for this change?:
Add GetDatasetFileCounts use case
Additional documentation:
None