-
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
109 - Add totalFilesCount to GetDatasetFiles use case #116
109 - Add totalFilesCount to GetDatasetFiles use case #116
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.
We need also to export the FilesSubset
model in the index.ts
file of the files folder, just as we do with the DatasetPreviewSubset
I addressed the change. But I'm worried about what would had happened if you hadn't noticed. I think that somehow we need some tests to test the package from the outside. To force the check on these index exports, and also ensure that the package functions properly when used as a library |
Yes. There is an open issue related to functional tests that would be useful for what you mention: #81 I just updated the issue to link to this comment. |
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, I forgot to submit this review.
Can you check the comment?
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
filesPayload.forEach(function (filePayload: any) { | ||
files.push(transformFilePayloadToFile(filePayload)); | ||
filesPayload.forEach(function (filesPayload: any) { |
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.
Why have you changed the name of the variable inside the function from singular to plural?
Now we have transformFilePayloadToFile(filesPayload)
which makes it a little confusing.
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.
My bad, I committed the change to restore the name
What this PR does / why we need it:
This PR introduces
totalFilesCount
to theGetDatasetFiles
use case, which is essential for pagination. This ensures consistency with the pagination implementation, as seen inDatasetPreviewSubset
.Which issue(s) this PR closes:
Related Dataverse PRs:
Special notes for your reviewer:
The implementation should be consistent with GetAllDatasetPreviews implementation, so just compare the code with that use case
Suggestions on how to test this:
Run integration tests or review GitHub action execution