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

Add unset on navigators #318

Merged
merged 1 commit into from
Dec 10, 2018
Merged

Add unset on navigators #318

merged 1 commit into from
Dec 10, 2018

Conversation

nlepage
Copy link
Member

@nlepage nlepage commented Dec 3, 2018

Description

Add unset operation on navigators, and use it in unset.

Issue : fix #305

@nlepage nlepage added this to the 2.0 milestone Dec 3, 2018
@nlepage nlepage self-assigned this Dec 3, 2018
unset() {
const copy = this.copy()

if (this.next.final)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is barely the same code as in the indexNav. Can we do something about it ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this is exactly the same code as in IndexNav, and we could mutualize it in their common ancestor BaseNav.
But for comprehension sake, I think it is better to have two copies of this method, one that uses an index which denotes an array access, and one that uses a key which denotes a property access.

@codecov-io
Copy link

codecov-io commented Dec 4, 2018

Codecov Report

Merging #318 into dev will decrease coverage by 0.49%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff            @@
##              dev     #318     +/-   ##
=========================================
- Coverage   97.17%   96.68%   -0.5%     
=========================================
  Files         107      108      +1     
  Lines         425      452     +27     
=========================================
+ Hits          413      437     +24     
- Misses         12       15      +3
Impacted Files Coverage Δ
packages/immutadot/src/nav/nav.js 87.5% <ø> (-4.17%) ⬇️
packages/immutadot/src/nav/propNav.js 100% <100%> (ø) ⬆️
packages/immutadot/src/core/unset.js 100% <100%> (ø) ⬆️
packages/immutadot/src/nav/baseNav.js 100% <100%> (ø) ⬆️
packages/immutadot/src/nav/finalNav.js 100% <100%> (ø)
packages/immutadot/src/nav/indexNav.js 94.44% <80%> (-5.56%) ⬇️
packages/immutadot/src/nav/propsNav.js 85% <87.5%> (+1.66%) ⬆️
packages/immutadot/src/nav/sliceNav.js 96.15% <87.5%> (-3.85%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27b9903...bbcb9ea. Read the comment docs.

frinyvonnick
frinyvonnick previously approved these changes Dec 7, 2018
@nlepage nlepage changed the base branch from enhance/316 to dev December 10, 2018 09:22
@nlepage nlepage merged commit 433a0c2 into dev Dec 10, 2018
@nlepage nlepage deleted the feature/305 branch December 10, 2018 09:31
nlepage added a commit that referenced this pull request Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants