Adds PersistentVector.mapi by njlr · Pull Request #186 · fsprojects/FSharpx.Collections · GitHub
Skip to content

Adds PersistentVector.mapi#186

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

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

Conversation

@njlr

@njlr njlr commented Nov 3, 2021

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

njlr commented Jan 28, 2022

Copy link
Copy Markdown
Contributor Author

Sorry to bump @gdziadkiewicz

@gdziadkiewicz

Copy link
Copy Markdown
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

njlr commented Nov 7, 2022

Copy link
Copy Markdown
Contributor Author

Sorry to bump!

@jackfoxy

jackfoxy commented Nov 7, 2022

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Collaborator

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

@dsyme

dsyme commented Nov 8, 2022

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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