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

Splitting out OrderedCollections #392

Closed
timholy opened this issue Jun 25, 2018 · 9 comments
Closed

Splitting out OrderedCollections #392

timholy opened this issue Jun 25, 2018 · 9 comments

Comments

@timholy
Copy link
Member

timholy commented Jun 25, 2018

There seems to be some interest in trimming down this package. I just split out the two ordered containers, OrderedDict and OrderedSet, in a candidate package called OrderedCollections and made 0.7 a requirement. (I also updated to iterate rather than using the legacy start/next/done.) I am hosting it on my own account for now but will move it to JuliaCollections if there is interest.

I did my best to preserve the git history while slimming it down as much as I had patience for. There's probably a lot of extra history there, but I thought that might be better than just starting fresh.

Let me know your thoughts.

@StephenVavasis
Copy link
Contributor

Tim,

I had the impression that the old code for OrderedDict had been orphaned, and I had already proposed to completely replace it. See #386 for my proposed replacement as well mine and other people's (e.g. @kmsquire) comments. However, it seems like you are willing to "adopt" the old code. If this is correct, then I will withdraw 386.

Please note that the old OrderedSet code has an undocumented getindex method (indexing with Ints). It is not clear to me how this method is supposed to interact with deletions. I suggest that this getindex method should either be removed or documented.

@timholy
Copy link
Member Author

timholy commented Jun 25, 2018

I just hadn't seen that. This was just an effort at trimming down dependencies. If folks want this split, I will spend time to look into your new implementation and the various associated issues.

Main question for now, though, is whether the split is a good thing.

@oxinabox
Copy link
Member

The split is a good thing.
Is OrderedCollections a too narrow though?
My thoughts were that it could go with all the other dict/set collections except the SortedCollections.

But if this resolution works, and is useful to people, then this works.
I don't think it is worth bikeshedding over, as I don't think it makes much of a maintenance difference.
Getting the split done anyway or the other will make a maintenance difference.

cf: #310

When you are happy with what you've got, and it is registered.
I think we can remove it from DataStructures.jl,
up the DataStructures.jl requirement to 0.7, and @reexport it.
it is that time now.

@timholy
Copy link
Member Author

timholy commented Jun 26, 2018

Thanks for the link. I understand why you might think to bundle more stuff together. However, as you noticed in that issue "Associative" is quite a grab-bag. From a purely conceptual viewpoint, I'm not even sure whether "Associative" is truly meaningful, in that e.g., Arrays can be thought of as associative containers with keys that obey certain properties. Features like "is sorting maintained?" seem like a much more fundamental separation among components.

Also, I confess that I have mixed enthusiasm for some of the other associative structures---many seem to be small increments in functionality that, often as not, I just implement directly when I need them. EDIT: the previous comment was meant to apply to the various Default* types, MultiDict, and Accumulator; the Sorted* types have a clear and obvious purpose as stand-alone data structures.

I've further improved the OrderedCollections repo to the point that it is mostly ready. @StephenVavasis, you're exactly right about getindex. I've started a discussion in #180 about how to handle that.

@oxinabox
Copy link
Member

I am now in agreement

@timholy
Copy link
Member Author

timholy commented Jun 26, 2018

Great. I'll wait a few days and see whether any objections arise, and if nothing too serious comes up I'll go ahead.

@tlnagy
Copy link

tlnagy commented Jun 29, 2018

Wait...is there a discussion of the rationale behind this move? This package doesn't seem that big to warrant it being broken up.

@oxinabox
Copy link
Member

@tlnagy #310

@oxinabox
Copy link
Member

oxinabox commented Aug 1, 2018

@timholy will you do the honours and remove OrderedCollections from DataStructures.jl, and. add it as a reexport?

(I'ld rather not spend times crossportign PRs)

timholy added a commit that referenced this issue Sep 19, 2018
timholy added a commit that referenced this issue Sep 19, 2018
timholy added a commit that referenced this issue Sep 19, 2018
timholy added a commit that referenced this issue Sep 19, 2018
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

No branches or pull requests

4 participants