Skip to content
This repository has been archived by the owner on Jan 6, 2025. It is now read-only.

Slice walking implementation #124

Merged
merged 11 commits into from
Nov 13, 2017
Merged

Slice walking implementation #124

merged 11 commits into from
Nov 13, 2017

Conversation

nlepage
Copy link
Member

@nlepage nlepage commented Nov 6, 2017

Description

Slice walking implementation

Issue : #94

@nlepage nlepage added this to the 0.4-alpha milestone Nov 6, 2017
@nlepage nlepage requested a review from frinyvonnick as a code owner November 6, 2017 18:07
@codecov-io
Copy link

codecov-io commented Nov 6, 2017

Codecov Report

Merging #124 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #124   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          74     74           
  Lines         247    259   +12     
  Branches       48     54    +6     
=====================================
+ Hits          247    259   +12
Impacted Files Coverage Δ
src/core/path.utils.js 100% <100%> (ø) ⬆️
src/core/apply.js 100% <100%> (ø) ⬆️
src/array/convertArrayMethod.js 100% <100%> (ø) ⬆️
src/util/lang.js 100% <100%> (ø) ⬆️

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 f97fe2c...ddf4c50. Read the comment docs.

@nlepage nlepage changed the title ✨ First naive implementation of slice walking 🚧 First naive implementation of slice walking Nov 6, 2017
@nlepage nlepage changed the title 🚧 First naive implementation of slice walking 🚧 Slice walking implementation Nov 6, 2017
@nlepage nlepage self-assigned this Nov 6, 2017
@nlepage nlepage force-pushed the feature/94 branch 6 times, most recently from a37d9c7 to bbb85da Compare November 12, 2017 22:04
@nlepage nlepage changed the base branch from master to enhance/convert-use-apply November 12, 2017 22:04
@nlepage nlepage changed the title 🚧 Slice walking implementation Slice walking implementation Nov 12, 2017
@nlepage
Copy link
Member Author

nlepage commented Nov 12, 2017

This isn't WIP anymore, I think we can merge this.
I added a new issue #129 to add more tests to apply.spec.js.

const copyArray = array => {
if (array === undefined || array === null) return []
if (isNil(array)) return []
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

const [prop, ...pathRest] = curPath

const value = callback(curObj, prop)
if (isSlice(prop)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👏


const newObj = copy(curObj, isArrayProp(prop))
let newObj = curObj
if (doCopy) newObj = copy(curObj, isIndex(prop))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need it ?

Copy link
Member Author

@nlepage nlepage Nov 13, 2017

Choose a reason for hiding this comment

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

When we are iterating on a slice, only one copy of the array is necessary, so this is to avoid several copies

if (isSlice(prop)) {
const [start, end] = getSliceBounds(prop, length(curObj))

const newArr = copy(curObj, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

the second param is not clear. Usually the second param in a copy function is for deep not asArray

Copy link
Member Author

Choose a reason for hiding this comment

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

As long as this is documented...

Copy link
Member Author

Choose a reason for hiding this comment

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

And this is private code

@nlepage nlepage changed the base branch from enhance/convert-use-apply to master November 13, 2017 08:18
@nlepage nlepage requested a review from charlyx November 13, 2017 08:25
* @private
* @since 0.4.0
*/
const getSliceBounds = ([start, end], length) => ([
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

* @since 0.4.0
*/
const length = arg => {
if (isNil(arg) || !isNaturalInteger(arg.length)) return 0
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@nlepage nlepage merged commit 33165c2 into master Nov 13, 2017
@nlepage nlepage deleted the feature/94 branch November 13, 2017 09:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants