review: Add TODOs to pure/List (ignore for merging) #195
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
|
✨ Documentation preview for 7798a18:
|
8 tasks
|
See #189. Containing some corrections. And more tests. |
ggreif
reviewed
Feb 28, 2025
| @@ -584,6 +597,7 @@ module { | |||
| /// Space: O(n) | |||
| /// | |||
| /// *Runtime and space assumes that `f` runs in O(1) time and space. | |||
| // TODO: now: make TR - was TR in base | |||
ggreif
reviewed
Feb 28, 2025
| @@ -616,6 +630,7 @@ module { | |||
| /// Runtime: O(n) | |||
| /// | |||
| /// Space: O(n) | |||
| // TODO: now: make TR - was TR in base (see replicate) | |||
ggreif
reviewed
Feb 28, 2025
| @@ -797,6 +817,7 @@ module { | |||
| /// Runtime: O(size) | |||
| /// | |||
| /// Space: O(size) | |||
| // TODO: make TR was TR in base | |||
There was a problem hiding this comment.
I only see toIter in legacy base, maybe named differently?
Found it: Iter.toList
ggreif
reviewed
Feb 28, 2025
| @@ -797,6 +817,7 @@ module { | |||
| /// Runtime: O(size) | |||
| /// | |||
| /// Space: O(size) | |||
| // TODO: make TR was TR in base | |||
There was a problem hiding this comment.
Suggested change
| // TODO: make TR was TR in base | |
| // TODO: now: make TR - was TR in base |
ggreif
reviewed
Feb 28, 2025
| @@ -302,11 +309,13 @@ module { | |||
| /// Runtime: O(size(l)) | |||
| /// | |||
| /// Space: O(size(l)) | |||
| // TODO: now make TR - append was TR in base | |||
There was a problem hiding this comment.
Suggested change
| // TODO: now make TR - append was TR in base | |
| // TODO: now: make TR - `append` was TR in base |
ggreif
reviewed
Feb 28, 2025
| public func concat<T>(list1 : List<T>, list2 : List<T>) : List<T> = switch list1 { | ||
| case null list2; | ||
| case (?(h, t)) ?(h, concat(t, list2)) | ||
| }; | ||
|
|
||
| // TODO: documentation |
ggreif
reviewed
Feb 28, 2025
| @@ -530,6 +541,7 @@ module { | |||
| /// Space: O(1) | |||
| /// | |||
| /// *Runtime and space assumes that `equalFunc` runs in O(1) time and space. | |||
| // TODO: rename equalFunc equalItem | |||
ggreif
reviewed
Feb 28, 2025
| @@ -554,6 +566,7 @@ module { | |||
| /// Space: O(1) | |||
| /// | |||
| /// *Runtime and space assumes that argument `compare` runs in O(1) time and space. | |||
| // TODO: rename compare compareItem | |||
ggreif
reviewed
Feb 28, 2025
| @@ -8,6 +8,8 @@ | |||
| /// import List "mo:base/List"; | |||
| /// ``` | |||
|
|
|||
| // TODO: future: all examples are meant to be self-contained actors, don't include common code either. | |||
| // TODO: future: many switches test for incommon null case first - reorder for perf. | |||
There was a problem hiding this comment.
yeah, I was doing this reordering on the fly
crusso
changed the title
ggreif
reviewed
Feb 28, 2025
| @@ -254,6 +259,7 @@ module { | |||
| /// Space: O(size) | |||
| /// | |||
| /// *Runtime and space assumes that `f` runs in O(1) time and space. | |||
| // TODO: now: make TR - was TR in base | |||
There was a problem hiding this comment.
this is also fixed in #189 (with test added)
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.
I took pass through pure List and added immediate and future TODOs.
The immediate ones are regressions vs base and I think should be fixed ASAP.
Others, marked future are for us to revisit at leisure (existing flaws we can fix later)