-
Notifications
You must be signed in to change notification settings - Fork 1
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
[ENH] Standardized query result files #392
Conversation
Reviewer's Guide by SourceryThis PR standardizes the query result files by reorganizing the column order and handling human-readable vs. machine-readable outputs more consistently. The changes primarily focus on the TSV file generation logic in the ResultContainer component and its corresponding tests. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
✅ Deploy Preview for neurobagel-query ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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 @rmanaem - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -145,104 +145,146 @@ function ResultContainer({ | |||
const tsvRows = []; | |||
const datasets = response.responses.filter((res) => download.includes(res.dataset_uuid)); | |||
|
|||
const isHumanFile = buttonIdentifier === 'cohort-participant'; | |||
if (buttonIdentifier === 'cohort-participant') { |
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.
issue (complexity): Consider extracting the export logic into a reusable configuration-driven function.
The export logic can be simplified by extracting the common pattern into a reusable function. Here's how:
interface FieldConfig {
header: string;
getValue: (res: any, subject?: any) => string;
protected?: string;
}
const exportConfigs = {
'cohort-participant': {
fields: [
{ header: 'DatasetName', getValue: (res) => res.dataset_name.replace('\n', ' ') },
{ header: 'SubjectID', getValue: (_, subject) => subject?.sub_id, protected: 'protected' },
// ... other fields
]
},
'other-export': {
fields: [
// ... field configurations
]
}
};
function generateTSVString(buttonIdentifier: string) {
if (!response) return '';
const config = exportConfigs[buttonIdentifier];
const datasets = response.responses.filter((res) => download.includes(res.dataset_uuid));
const tsvRows = [config.fields.map(f => f.header).join('\t')];
datasets.forEach(res => {
if (res.records_protected) {
tsvRows.push(config.fields.map(f => f.protected || 'protected').join('\t'));
} else {
res.subject_data.forEach(subject => {
tsvRows.push(config.fields.map(f => f.getValue(res, subject)).join('\t'));
});
}
});
return tsvRows.join('\n');
}
This approach:
- Eliminates code duplication
- Makes field definitions and differences between export types explicit
- Easier to maintain and extend with new export types
- Reduces chance of bugs when making changes
Bumps [eslint-plugin-react-refresh](https://github.com/ArnaudBarre/eslint-plugin-react-refresh) from 0.4.14 to 0.4.16. - [Release notes](https://github.com/ArnaudBarre/eslint-plugin-react-refresh/releases) - [Changelog](https://github.com/ArnaudBarre/eslint-plugin-react-refresh/blob/main/CHANGELOG.md) - [Commits](ArnaudBarre/eslint-plugin-react-refresh@v0.4.14...v0.4.16) --- updated-dependencies: - dependency-name: eslint-plugin-react-refresh dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [@vitest/ui](https://github.com/vitest-dev/vitest/tree/HEAD/packages/ui) from 2.1.5 to 2.1.8. - [Release notes](https://github.com/vitest-dev/vitest/releases) - [Commits](https://github.com/vitest-dev/vitest/commits/v2.1.8/packages/ui) --- updated-dependencies: - dependency-name: "@vitest/ui" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Modified `build_docker_nightly` workflow * Set `GH_TOKEN` * A different approach * Use shortened sha * Updated `build_docker_on_release` workflow * Updated `deploy` workflow * Removed fetching version from GitHub * Fixed typo * Reverted changes made to `deploy` workflow * Added `bump_version` workflow * Updated `bump_version` workflow * Added the labeled type * Test different auto commands * comment out the label condition * Try canary command * Added github token * Let's try commands * let's try this solution * Removed couple steps * Set the version_schema * Remove prerelease_suffix * Removed default field * Trying the latest version * seems silly but worth a try * Installed auto as a dev dependency * Bumped version in `package` files * Let's try using npm * Added GH_TOKEN * Installed plugins * Grab the last line * Installed the npm plugin explicitly * Revert "Installed the npm plugin explicitly" This reverts commit 5def864. * Revert "Grab the last line" This reverts commit dfcfccb. * Revert "Installed plugins" This reverts commit ca0c485. * Revert "Added GH_TOKEN" This reverts commit f084501. * Revert "Let's try using npm" This reverts commit 602c6a2. * Revert "Bumped version in `package` files" This reverts commit 3a99f82. * Revert "Installed auto as a dev dependency" This reverts commit 5b9b98d. * Going back to version * Let's see the output * Read in a separate step * Let's try this * try again * Try verbose logs * Really? * ok does this work too? * Getting there * Forgot the GH_TOKEN again * Fixed the missing version * Let's run on pull_request * GH_TOKEN added * Almost there * Try a fix for committing to the same branch * figuring out the right syntax * an alternative appraoch * Let's see what's in there * Let's check this one out * This should do it * It's possible we didn't even needed the branch name * Try this syntax * Bumped version to v0.7.2 * Added a condition to check diff before commiting * Refactor * Added a condition to skip the workflow when increment is empty * Small refactor * Removed the explicit condition and used label condition * Bumped version to v0.7.2 * Updated `pull_request_template` * Reverted changes made to `build_docker_on_release` * Fixed typo --------- Co-authored-by: Neurobagel Bot <neurobagel-bot[bot]@users.noreply.github.com>
Hey @rmanaem I think you have the PR target mixed up here The diff also looks a little funny. Would you mind checking what is happening here and then let me know? |
query-tool-results
files neurobagel_examples#47Checklist
This section is for the PR reviewer
[ENH]
,[FIX]
,[REF]
,[TST]
,[CI]
,[MNT]
,[INF]
,[MODEL]
,[DOC]
) (see our Contributing Guidelines for more info)skip-release
(to be applied by maintainers only)Closes #XXXX
query-tool-results
files in the neurobagel_examples repo have been regeneratedFor new features:
For bug fixes:
Summary by Sourcery
Standardize the format of query result files by updating the headers and data structure for TSV files, and update tests to ensure correct data mapping and validation.
Enhancements:
Tests: