search

GenieFramework / StippleUI.jl

StippleUI is a library of reactive UI elements for Stipple.jl.
MIT License
82 stars 15 forks source link

Jdb/tables fix #122

Open jeremiedb opened 7 months ago

jeremiedb commented 7 months ago

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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

@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 commented 2 months ago

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

jeremiedb commented 2 months ago

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 commented 4 days ago

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