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

jsapi: fix null check for optional color param #2521

Merged
merged 1 commit into from
Jun 18, 2022

Conversation

nbauernfeind
Copy link
Member

@nbauernfeind nbauernfeind commented Jun 13, 2022

This just fixes some whining when an optional parameter is not specified.

@nbauernfeind nbauernfeind added this to the Jun 2022 milestone Jun 13, 2022
@nbauernfeind nbauernfeind self-assigned this Jun 13, 2022
@mattrunyon
Copy link
Contributor

mattrunyon commented Jun 13, 2022

I get the same error on the docs example with this fix

I think the issue Colin and I found originally was the tables get bound in a different order or 1 table gets bound as both.

We set a breakpoint and inspected and saw that table 0 was table 1 and table 1 was table 0, or table 0 was both table 0 and table 1 (I don't remember which, but I think it was swapped). This caused the issue with findColumn because the column name is from table 1, but looking up on table 0.

Copy link
Contributor

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

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

Still getting the error. I think it is more in depth and has to do with using a HashSet or HashMap and losing order somewhere

@niloc132
Copy link
Member

Agreed, I don't see how this can fix the issue at hand (though it is likely a bug in and of itself).

@mattrunyon mattrunyon self-requested a review June 16, 2022 16:44
mattrunyon
mattrunyon previously approved these changes Jun 16, 2022
@nbauernfeind nbauernfeind merged commit aa78aee into deephaven:main Jun 18, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jun 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants