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

Jdb/tables fix #122

Merged
merged 7 commits into from
Nov 14, 2024
Merged

Jdb/tables fix #122

merged 7 commits into from
Nov 14, 2024

Conversation

jeremiedb
Copy link
Contributor

Fix #120 #121
This PR overrides the label contructor in active_columns and DataTableOptions with vanilla string.
As for the best way forward, my suggestion would be to rely on an optional user specified vector of columnnames, but I'd definitly prefer that adhoc modifiers like uppercasing and substitution ("_" => "").
Also, since DataFrame supports the handling of non regular variable names (ex: DataFrame("Nice Name" => 1:2)), I think it makes the scenario where raw column names from source data are expected as labels (at least by default) quite reasonable. From a user perspective, I think it's easier to reason if there isn't an additonal layer of Tables Interface that acts upon the source table, beyond the well established Tables.jl interface.

Regardng the fix for adding back filter support, the only thing I'm not fully sure is whether this may interfere with the server side filtering support, as I'm not clear whether a server side filtering query would end up actually doing the client side filtering on top of it.

@hhaensel
Copy link
Member

@essenciary @PGimenez I like this PR. The existence of clean_label is a bit surprising.
Do you think, people will complain if we remove upcasing?

@essenciary
Copy link
Member

essenciary commented Dec 12, 2023

@hhaensel Yes, it probably makes sense to remove the automatic use of label_clean if it causes unexpected/undesired results. The users can call it explicitly if they want. It's probably a breaking change but should not have other impacts than aesthetics.

@jeremiedb I still need to fix the filtering, I'll get to it soon. It doesn't make sense for server side filtering to also run client side filtering as the server side filtering does the same thing (the client side will have nothing to filter). Maybe it makes sense to have an option to decide between server side and client side filtering.

@essenciary
Copy link
Member

@jeremiedb currently tagging a new version to address the filtering issue. Once that is confirmed to work as expected, we can make a decision on the label_clean proposal in this PR.

@hhaensel
Copy link
Member

@essenciary shouldn't we merge this?
Server-side filtering has been addressed already, right?
@jeremiedb Do you still want to add a change?

@jeremiedb
Copy link
Contributor Author

jeremiedb commented Apr 19, 2024

I'd be happy to the PR merged as it is currently, which keeps the name as extracted from the Tables API.

Regarding if it's a breaking change, the label_clean functionnality was initially introduced in v0.22.9 so, at least historically, it had not been considered breaking https://github.com/GenieFramework/StippleUI.jl/blob/v0.22.9/src/Tables.jl.

And yes the filtering functionnality now works very smoothly, thanks!

@hhaensel
Copy link
Member

@essenciary, @PGimenez I think we should merge this PR. The reasoning is still valide and there's no other refactoring pending anynmore.

@jeremiedb
Copy link
Contributor Author

could it be merged?

@essenciary
Copy link
Member

Oops, sorry about this @jeremiedb
@hhaensel please merge if/when you see fit.

@hhaensel hhaensel merged commit c76e68a into GenieFramework:master Nov 14, 2024
7 of 10 checks passed
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.

Table filtering stopped working from v0.22.9
3 participants