-
Notifications
You must be signed in to change notification settings - Fork 26
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
Revert to using datasetList in jobParams #1616
base: release-jobs
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request reverts the job parameters structure to use a 'datasetList' instead of 'datasetIds'. It also fixes a bug in the archiving service that was preventing job creation from the frontend. The changes primarily affect how datasets and their associated files are handled in job creation and processing. Updated class diagram for Job parametersclassDiagram
class Job {
+String createdBy
+Date createdAt
+String type
+JobParams jobParams
}
class JobParams {
+List~Dataset~ datasetList
}
class Dataset {
+String pid
+List~String~ files
}
JobParams --> Dataset
Dataset "1" -- "*" String : files
note for JobParams "Reverted to using datasetList instead of datasetIds"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @despadam - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider conducting a comprehensive review of all job-related code to ensure consistent handling of the new datasetList structure throughout the codebase.
- The removal of createdBy and createdAt fields in ArchivingService.ts seems unintentional. Please verify if this was intended and adjust accordingly to maintain consistent job metadata.
- While the new structure provides more granular control, ensure that empty file lists are handled consistently across all affected components (e.g., using empty arrays vs. omitting the files property).
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Hi @despadam! 👋
@sourcery-ai is now installed on this repository.
We found this recent PR of yours and reviewed it to show you what Sourcery can do.
If you want to review another PR, just comment with @sourcery-ai review
✨
datasetList: [ | ||
{ | ||
pid: this.datasetPid, | ||
files: this.getSelectedFiles(), |
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.
suggestion (performance): Store result of getSelectedFiles() in a variable
To avoid potential multiple calls to getSelectedFiles(), consider storing its result in a variable before using it in the job parameters.
const selectedFiles = this.getSelectedFiles();
datasetList: [
{
pid: this.datasetPid,
files: selectedFiles,
},
],
jobParams
:src/app/datasets/archiving.service.ts
that was not allowing job creation from the frontend.Summary by Sourcery
Revert to using 'datasetList' in job parameters and fix a bug in the archiving service to allow job creation from the frontend.
Bug Fixes:
Enhancements: