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

Replace fun input with ownership #22

Closed
wants to merge 15 commits into from

Conversation

gkorland
Copy link
Contributor

When a value is passed by value and not reference the user can return the original value and avoid clone()

@freestrings
Copy link
Owner

@gkorland Very thanks.

But there are compile errors in the webassembly sub project. If you compile in the wasm directory, you will see the error.

freestrings added a commit that referenced this pull request Aug 15, 2019
@freestrings
Copy link
Owner

freestrings commented Aug 15, 2019

merged manually but not mark as merged.

freestrings added a commit that referenced this pull request Aug 15, 2019
@gkorland
Copy link
Contributor Author

@freestrings sorry I was off for a week, I'm not sure I follow, do you need me to reopen this PR?

@gkorland
Copy link
Contributor Author

@freestrings please check my last commit, I think this is the right way to add a way to let the user remove and not only replace with Null on delete

@freestrings
Copy link
Owner

@gkorland please check

  • failed cargo test
  • rebase this pr
  • add unit test for None return

since the function parameter type and return type have changed, I believe it seems better to release it in version 0.3.x that i am planning around mid next month.

@gkorland
Copy link
Contributor Author

@freestrings done

@gkorland
Copy link
Contributor Author

Fix #8 allows return None on replace --> remove path --> delete the key

@freestrings
Copy link
Owner

@gkorland Thanks.
there is confict with master. it can not be merged. you should rebase this branch.

@gkorland
Copy link
Contributor Author

gkorland commented Aug 27, 2019

@freestrings It says

This branch has no conflicts with the base branch

@freestrings
Copy link
Owner

freestrings commented Aug 28, 2019

@gkorland
Since Github is displaying the "This branch cannot be rebased due to conflicts" message, merging manually from the command line may not appear as merged PR.

Screenshot from 2019-08-28 22-41-02

@gkorland
Copy link
Contributor Author

@freestrings is it working now?

@freestrings
Copy link
Owner

@gkorland No. It seems better to merge it manually and close this PR. what do you think?

@gkorland
Copy link
Contributor Author

@freestrings go for it, it does seem like we're wasting time on git tricks...

freestrings added a commit that referenced this pull request Aug 29, 2019
freestrings added a commit that referenced this pull request Aug 29, 2019
@freestrings
Copy link
Owner

@gkorland this PR merged manually.

@gkorland gkorland deleted the reaplce_with_ownership branch August 29, 2019 15:16
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.

None yet

2 participants