Skip to content

Adds PersistentVector.mapi#186

Merged
gdziadkiewicz merged 1 commit intofsprojects:masterfrom
njlr:feature-persistent-vector-mapi
Nov 8, 2022
Merged

Adds PersistentVector.mapi#186
gdziadkiewicz merged 1 commit intofsprojects:masterfrom
njlr:feature-persistent-vector-mapi

Conversation

@njlr
Copy link
Contributor

@njlr njlr commented Nov 3, 2021

This mirrors List.mapi and Seq.mapi in the core library.

for i in 1..300 do i * 2 |> Expect.equal "map" a.[i-1] }

test "vector should allow mapi" {
let vector = ref PersistentVector.empty
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is ref cell needed? Can't we handle it using mutable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied from the test above "vector with 300 elements should allow pop"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to change it to mutable (I prefer that too) but was trying to follow style of existing code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed that the rest of the file approaches it like that. I would leave it as it is and potentially tackle all of the ref cells in tests in separate PR. @jackfoxy what's your opinion on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK to merge?

@njlr
Copy link
Contributor Author

njlr commented Jan 28, 2022

Sorry to bump @gdziadkiewicz

@gdziadkiewicz
Copy link
Collaborator

Feel free to bump me as much as necessary. The problem is that I don't have merge rights for this repo. @forki @mausch @jackfoxy @fsprojectsgit are you able to help with reviewing and merging this PR?

@njlr
Copy link
Contributor Author

njlr commented Nov 7, 2022

Sorry to bump!

@jackfoxy
Copy link
Contributor

jackfoxy commented Nov 7, 2022

@dsyme please remove me from this and any other FsProjects repos I may be as a maintainer.
There are at least a couple of PRs in this repo awaiting review. I don't know if there are any active maintainers on this project now.

@gdziadkiewicz
Copy link
Collaborator

I'm willing to contribute as an maintainer if an maintainer is needed.

@dsyme
Copy link
Contributor

dsyme commented Nov 8, 2022

@gdziadkiewicz Lovely, I'll add you as maintainer in this one

@jackfoxy I'll remove you - many thanks for everything you've contributed to this and the other projects over time!!

@gdziadkiewicz gdziadkiewicz merged commit 743e91c into fsprojects:master Nov 8, 2022
@gdziadkiewicz
Copy link
Collaborator

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

Successfully merging this pull request may close these issues.

4 participants