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

feat(bool): add boolean type #139

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

thanh-nguyen-dang
Copy link
Contributor

Jira Ticket: PXP-xxxx

  • Remove this line if you've changed the title to (PXP-xxxx): <title>

New Features

Breaking Changes

Bug Fixes

Improvements

Dependency updates

Deployment changes

Copy link
Contributor

@mfshao mfshao left a comment

Choose a reason for hiding this comment

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

See my comment

I would say I'd still perfer to make Tube be backward-compatible, especially considering the new Tube release just bumps up its patch version, rather than making a breaking change into Guppy and/or Portal

@@ -11,6 +11,7 @@ const esgqlTypeMapping = {
byte: 'Int',
double: 'Float',
float: 'Float',
boolean: 'Boolean',
Copy link
Contributor

Choose a reason for hiding this comment

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

how about just treat boolean as string for gql types?

either way, we will need to test before merging, especially to test use new Tube do ETL, and then see if Guppy/Portal can work (data explorer/study viewer)

One concern I have is that in data explorer we need to verify if we put a boolean field in filters, when selecting the filter, does it will send a string value back to guppy and causes problems. This can be testing in qa-covid19

Copy link
Contributor Author

@thanh-nguyen-dang thanh-nguyen-dang Apr 21, 2022

Choose a reason for hiding this comment

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

it is tested in qa-niaid @mfshao . And we should use boolean if graphql supports boolean.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want see it tested in one env for one feature and after merging it breaks another feature in another env. If we know it might potential be dangerous, we need to do more testing and verify things are ok, or fix more before merging. Don't rush to merge into master

@ocshawn
Copy link
Contributor

ocshawn commented Jan 6, 2023

@thanh-nguyen-dang is this still an issue? can we get someone to test it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants