-
Notifications
You must be signed in to change notification settings - Fork 71
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
[Draft] Collections API Rework and Feature Enhancements #1337
Draft
Jason94
wants to merge
62
commits into
coalton-lang:main
Choose a base branch
from
Jason94:collections-rework
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jason94
force-pushed
the
collections-rework
branch
from
December 30, 2024 15:56
50946f3
to
c3321a9
Compare
Includes Collection and MutableCollection
Jason94
force-pushed
the
collections-rework
branch
from
January 4, 2025 05:01
da355f6
to
0e58929
Compare
Jason94
force-pushed
the
collections-rework
branch
4 times, most recently
from
January 10, 2025 20:04
62517fa
to
539ac1f
Compare
Jason94
force-pushed
the
collections-rework
branch
from
January 10, 2025 20:06
539ac1f
to
c0921f0
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This draft PR is a start to implementing the proposal in this discussion. The code mostly matches the API described in the proposal, with small differences as development arose.
Not mentioned in the discussion, but I've also started on a macro-based test suite to verify that all collections conform to the Collections contract.
Will resolve: #1303
Depends on: #1342
Example Code
The following snippet demonstrates usage of the new API. Note the creation of functions generic over
LinearCollection
s, calling the same functions directly on Lists and Vectors, and that the entire collections API is able to be imported directly through coalton-prelude.Progress
Goals for this pull request:
Work that I believe should be left to a future PR:
Open Questions
I'm posting here to get general feedback on the API as I continue, and also to request input on a few specific questions.
1. Start with List & Vector, or go all in?
OrdMap and Seq will probably require substantial work to get them to feature parity with the other Collections, since the underlying data structures are much more complicated. (AT least, if done efficiently) HashTable might be easier. List and Vector are basically already done. We could release the Collections API just covering List and Vector initially, and then release PR's to bring the others up to parity afterwards. Or we could wait to release them all at once.
2. How much effort to put into backwards compatibility?
The new API is a big departure in behavior, function naming, and packaging. It will certainly break old code. It will probably require broad but safe refactors (mostly renaming functions) for any existing Coalton code. The discussion proposed a deprecation warning scheme. The current PR doesn't go that far, but does at least preserve the old packages as duplicate files in the meantime.
Release Notes
coalton-library/list
andcoalton-library/vector
still exist, and are largely unchanged, so that code can continue using the old function names & signatures.coalton-library/collections/classes
package adds a few dozen new function symbols into prelude, which could break existing code if they're not shadowed.vector:pop!
andvector:push!
- Previouslylist:push
pushed to the front andvector:push!
pushed to the end. The new API has been unified so that any unqualified function manipulates the head (push
,pop!
, etc) and end functions are specified (push-end
,pop-end!
, etc).add
andadd!
are new functions on theCollection
typeclass that requires the element be added, but it doesn't specify where, to allow the datatype to add it in the most efficient way (and to accommodate non-linear collections like sets).add
/add!
unless you care about the ordering.vector:pop!
orvector:push!
needs to switch tovector:pop-end!
andvector:add!
/vector:push-end!
.collections-library/list
is no longer re-exported intocoalton-prelude
. Most of the functions from that package have an exact or close analogue incoalton-library/collections
, which is re-exported from prelude.:use
collections-library/collections/immutable/list
without any issues to restore those functions to scope.