StippleUI.jl icon indicating copy to clipboard operation
StippleUI.jl copied to clipboard

Jdb/tables fix

Open jeremiedb opened this issue 2 years ago • 6 comments
trafficstars

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.

jeremiedb avatar Nov 17 '23 17:11 jeremiedb

@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?

hhaensel avatar Dec 11 '23 22:12 hhaensel

@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 avatar Dec 12 '23 08:12 essenciary

@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.

essenciary avatar Jan 03 '24 14:01 essenciary

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

hhaensel avatar Apr 19 '24 12:04 hhaensel

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!

jeremiedb avatar Apr 19 '24 13:04 jeremiedb

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

hhaensel avatar Jun 30 '24 19:06 hhaensel