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

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
@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 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.

4 participants