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

Closes #26 - Enable Filtering on async tables #27

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

Conversation

jekelsc
Copy link

@jekelsc jekelsc commented Jun 21, 2019

Enables default ag-grid style filtering on async tables by creating a filter function from filterModel data.

Enables default ag-grid style filtering on async tables by creating a filter function from filterModel data.
Minor adjustment to return the invokelatest wrapped filter on the first use. The world will be ready for the actual function on subsequent uses.
Added code to sanitize column access and partially specified ranges. Optimize dates via preparsing.
Handle column names and search text with quote characters more defensively.
@pfitzseb
Copy link
Member

Thanks for taking a stab at this!
This is looking pretty good, but imho it would be much better to construct expressions directly instead of constructing strings and parsing those. Can you try to make that change?
Would also be great to have some tests for the Julia side at least (integration tests for both JS and Julia code aren't super easy to set up) -- both unit tests for some of the new functions and a test or two for the whole filter pipeline.

Updated to generate expressions directly rather than parsing a string.
@jekelsc
Copy link
Author

jekelsc commented Jun 24, 2019

I might have some time to add tests for the filterModel to expression logic in the next couple of days. Feel free to try the latest version and give feedback as it is quite useful. Can't wait to use it in my day job!

@code_typed shows functions like build_date returning type Union{Missing, Expr} when not using OptionaExpr, but Any after a type assertion when using OptionalExpr. Type inference is better than type assertion if the code is correct I guess.
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.

2 participants