-
Notifications
You must be signed in to change notification settings - Fork 324
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
Dynamic widget improvements to Table.filter
.
#12032
base: develop
Are you sure you want to change the base?
Conversation
distribution/lib/Standard/Table/0.0.0-dev/src/Internal/Widget_Helpers.enso
Outdated
Show resolved
Hide resolved
distribution/lib/Standard/Table/0.0.0-dev/src/Internal/Widget_Helpers.enso
Outdated
Show resolved
Hide resolved
builder.append (Option "Equals" "..Equal" [["to", column_chooser]]) | ||
builder.append (Option "Not Equals" "..Not_Equal" [["to", column_chooser]]) | ||
|
||
if column_value_type != Value_Type.Boolean then |
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.
We have a method on Value_Type
that checks this condition more precisely, would be better to use it:
if column_value_type != Value_Type.Boolean then | |
if column_value_type.has_ordering then |
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.
Want to exclude Boolean here as well (it has an ordering) but have updated.
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.
Ahh, I forgot that Boolean has ordering then. Hmm. Perhaps then column_value_type.has_ordering && column_value_type != Value_Type.Boolean
to exclude the boolean explicitly?
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.
This is really great, I'm very happy we are finally getting this improvement.
I added a few small suggestions.
As expected, I'm slightly concerned with the lack of tests. Is there no way to mock the cache
? If not possible, do we have some ticket tracking this? As it would be good to have at least some tests for this at least at some point.
I guess many of our widgets do not really have tests, but the logic here is more complicated than before, so it would be great to have at least some sanity checks.
Pull Request Description
2025-01-09_15-53-41.mp4
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.