Skip to content
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] Made requirement of age annotation more prominent #830

Merged
merged 7 commits into from
Dec 12, 2024
Merged

Conversation

rmanaem
Copy link
Contributor

@rmanaem rmanaem commented Dec 12, 2024

Checklist

This section is for the PR reviewer

  • PR has an interpretable title with a prefix ([ENH], [FIX], [REF], [TST], [CI], [MNT], [INF], [MODEL], [DOC]) (see our Contributing Guidelines for more info)
  • PR links to GitHub issue with mention Closes #XXXX
  • Tests pass
  • Checks pass

For new features:

  • Tests have been added

For bug fixes:

  • There is at least one test that would fail under the original bug conditions.

Summary by Sourcery

Enhancements:

  • Remove the 'int' term from the store configuration and related test cases.

Copy link

sourcery-ai bot commented Dec 12, 2024

Reviewer's Guide by Sourcery

This PR removes the integer value transformation option and enhances the UI to make age annotation requirements more prominent. The changes primarily involve removing the 'int' transformation option from various store and test files, and updating UI text to better indicate required fields and incomplete annotations.

Class diagram for store transformation options

classDiagram
    class Store {
        - bounded
        - euro
        - float
        - iso8601
    }
    note for Store "Removed 'int' transformation option"
Loading

File-Level Changes

Change Details Files
Removed integer value transformation option from the system
  • Removed 'int' option from transformation options list
  • Deleted integer value type definition and configuration
  • Updated test assertions to reflect removal of 'int' option
cypress/unit/store-getter-getTransformOptions.cy.js
store/index.js
cypress/unit/store-getter-getContinousJsonOutput.cy.js
Enhanced UI text for better clarity on age annotations
  • Added asterisk to age format selection dropdown text to indicate required field
  • Updated incomplete annotations message wording
  • Modified column annotation status message for better clarity
components/annot-continuous-values.vue
pages/download.vue

Assessment against linked issues

Issue Objective Addressed Explanation
#817 Make sections of annotation page/components collapsable and collapsed by default (except explanation and review) The diff does not show any changes related to making sections collapsable or changing their default state
#817 Change phrasing from 'missing annotations' to 'incomplete annotations'
#817 Add a label and required star (*) to the transformation dropdown

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@rmanaem rmanaem added the pr-patch Incremental feature improvement, will increment patch version when merged (0.0.+1) label Dec 12, 2024
Copy link

netlify bot commented Dec 12, 2024

Deploy Preview for neurobagel-annotator ready!

Name Link
🔨 Latest commit 313d333
🔍 Latest deploy log https://app.netlify.com/sites/neurobagel-annotator/deploys/675b55a61393f9000833ecbc
😎 Deploy Preview https://deploy-preview-830--neurobagel-annotator.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

@sourcery-ai sourcery-ai bot left a 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 - here's some feedback:

Overall Comments:

  • Please explain the rationale for removing the integer value type option. This appears to be a breaking change that should be documented.
  • The PR description should be updated to reflect all significant changes, including the removal of integer type support.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@surchs surchs self-requested a review December 12, 2024 21:01
Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rmanaem, looks good.

It seems you also have the diff from #829 in here. Assuming you branched off of that feature-branch? Just double check that you're not getting into a diff-tug-of-war here.

🧑‍🍳

@rmanaem rmanaem merged commit fe3f035 into main Dec 12, 2024
9 checks passed
@rmanaem rmanaem deleted the enh-817 branch December 12, 2024 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-patch Incremental feature improvement, will increment patch version when merged (0.0.+1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make required age annotation more obvious
2 participants