-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add Selector
abstract type for ecosytem compat, and rethink Between
#41
Comments
I agree we can make this change. This would be breaking for DataAPI.jl, so 2.0 release would be needed, which is a bit painful given then number of dependants https://juliahub.com/ui/Packages/DataAPI/3a8mN/1.8.0?t=2. However, I assume that only DataFrames.jl uses In DataFrames.jl we could allow both 1 and 2 release of DataAPI.jl and appropriately handle both cases. AN ALTERNATIVE that would be non-breaking would be to define CC @nalimilan |
If it's less breaking DimensionalData.jl can change to 2 fields, there is a breaking version bump coming up soon anyway, and it wont affect behavior as the constructor already accepts either The reason for using 1 field was so that all |
DD also defines rebuild(x; kw...) = ConstructionBase.setproperties(x, (; kw...)) So that: rebuild(sel; val=2 .* val(sel)) Or similar will just work, even when a |
This is what I have feared that there might be such cases. Let us wait for @nalimilan to comment in the first place what he thinks about merging the definitions and then we will decide what path should be taken to merge them (as maybe we can agree that DataAPI.jl definition is "internal" and changing it is non-breaking as long as we keep the external API untouched). |
Merging definitions sounds like a good idea, but tagging DataAPI 2.0 now is a no-go IMO. If we really want to switch to a single field, we could define appropriate constructors and That said, I don't really get the value of being able to do |
AxisKeys.jl takes Its |
One small consideration to also mention is Line 196 in 92b0def
|
@nalimilan Multiplying values by 2 was a bad example... practically it's more like swapping out their contents, e.g. a But maybe DD can also move to using @mcabbott yes |
DimensionalData.jl defines a
Selector
abstract type and selectorsBetween
,Near
,At
,Where
, andContains
.AxisKeys.jl also defines
Selector
andNear
andInervals
selectors (ping @mcabbott)It would be useful to standardise these so we can all use the simplest words without clashes, and this is probably the place for that.
We could define
Selector
here, and makeBetween <: Selector
.Near
could also move here to avoid the (maybe less common) clashes between AxisKeys.jl and DimensionalData.jl, if @mcabbott is also interested in sharing these types.Between
in DD holds a 2-tuple of any types, while here it has two fields limited toUnion{Int,Symbol}
.The text was updated successfully, but these errors were encountered: