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

Adding array split(size:) method #199

Merged
merged 4 commits into from
Jul 18, 2017
Merged

Adding array split(size:) method #199

merged 4 commits into from
Jul 18, 2017

Conversation

LucianoPAlmeida
Copy link
Member

Checklist

  • I checked the Contributing Guidelines before creating this request.
  • New extensions are written in Swift 3.
  • New extensions support iOS 8.0+ / tvOS 9.0+ / macOS 10.10+ / watchOS 2.0+.
  • I have added tests for new extensions, and they passed.
  • All extensions have a clear comments explaining their functionality, all parameters and return type in English.
  • All extensions are declared as public.

@codecov
Copy link

codecov bot commented Jul 17, 2017

Codecov Report

Merging #199 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #199      +/-   ##
==========================================
+ Coverage   92.84%   92.89%   +0.04%     
==========================================
  Files          85       85              
  Lines        4837     4871      +34     
==========================================
+ Hits         4491     4525      +34     
  Misses        346      346
Impacted Files Coverage Δ
Tests/FoundationTests/ArrayExtensionsTests.swift 98.11% <100%> (+0.19%) ⬆️
...ources/Extensions/Foundation/ArrayExtensions.swift 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 b441305...81250f7. Read the comment docs.

///
/// - Parameters:
/// - size: The size of the slices to be returned.
public func split(size: Int) -> [ArraySlice<Element>]? {
Copy link
Member

@SD10 SD10 Jul 17, 2017

Choose a reason for hiding this comment

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

I'm a little concerned about the memory implications of returning an ArraySlice here. I think returning an Array would be a safer API for the user. Is there a reason you chose to return an ArraySlice?

Copy link
Member

@SD10 SD10 Jul 17, 2017

Choose a reason for hiding this comment

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

@LucianoPAlmeida
I'm also thinking this method may be better named as group(by size: Int). I vaguely remember discussing this when adding the groupByKey extension recently.

Copy link
Member Author

Choose a reason for hiding this comment

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

@SD10 Hey man :)
About the memory implications, I think it's the kind of extension that the people who use knows the concerns about ArraySlice. But i don't know, may it's something to consider.
I just return the ArraySlice because I wanted to follow the same, pattern of the native mehtods that do similar stuff like split(whereSeparator: (Element) throws -> Bool)-> [ArraySlice<Element>] and by the way the name split is also to follow the pattern of this methods.
I thought about slices(ofSize:) or split(ofSize:), let me know what you think?

public func split(size: Int) -> [ArraySlice<Element>]? {
//Inspired by: https://lodash.com/docs/4.17.4#chunk
guard size > 0, !isEmpty else { return nil }
var value : Int = 0
Copy link
Member

Choose a reason for hiding this comment

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

Type inference 🤔

func testSplitSize() {

// A slice with value zero
var array : [String] = [ "james", "irving", "jordan", "jonshon", "iverson", "shaq"]
Copy link
Member

Choose a reason for hiding this comment

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

Type inference 😅

CHANGELOG.md Outdated
@@ -34,7 +34,7 @@ N/A
- New **Array** extensions
- added `groupByKey` to group the elements of the array by key in a dictionary. [#181](https://github.com/SwifterSwift/SwifterSwift/pull/181) by [@LucianoPAlmeida](https://github.com/LucianoPAlmeida)
- added `forEach(slice:body:)` to iterate by specified slice size and call a closure. [#194](https://github.com/SwifterSwift/SwifterSwift/pull/194/files) by [@LucianoPAlmeida](https://github.com/LucianoPAlmeida)

   - added `split(size:)` to split in an array of slices of a size. [#199](https://github.com/SwifterSwift/SwifterSwift/pull/199) by [@LucianoPAlmeida](https://github.com/LucianoPAlmeida)
Copy link
Member

Choose a reason for hiding this comment

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

Thank you 👍

@LucianoPAlmeida LucianoPAlmeida merged commit 981fc50 into SwifterSwift:master Jul 18, 2017
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.

2 participants