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

Added stablePartition by condition array method #367

Merged
merged 7 commits into from
Jan 18, 2018

Conversation

neoneye
Copy link
Contributor

@neoneye neoneye commented Jan 16, 2018

Checklist

  • I checked the Contributing Guidelines before creating this request.
  • New extensions are written in Swift 4.
  • 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.
  • I have added a changelog entry describing my changes.

@neoneye
Copy link
Contributor Author

neoneye commented Jan 16, 2018

This function is inspired by Ruby's partition function, http://ruby-doc.org/core-2.5.0/Enumerable.html#method-i-partition

The name partition is already taken by the Swift Array class. The second best name I could come up with is: stablePartition, since the element ordering is preserved.

@neoneye neoneye changed the title Adding stablePartition by condition array method Added stablePartition by condition array method Jan 16, 2018
@SwifterSwiftBot
Copy link

SwifterSwiftBot commented Jan 16, 2018

1 Warning
⚠️ Consider adding tests for new extensions or updating existing tests for a modified SwifterSwift extension
1 Message
📖 Thank you for submitting a pull request to SwifterSwift. The team will review your submission as soon as possible.

Generated by 🚫 Danger

@codecov
Copy link

codecov bot commented Jan 16, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #367      +/-   ##
==========================================
+ Coverage   90.68%   90.73%   +0.04%     
==========================================
  Files          53       53              
  Lines        2470     2482      +12     
==========================================
+ Hits         2240     2252      +12     
  Misses        230      230
Flag Coverage Δ
#ios 90.73% <100%> (+0.04%) ⬆️
#osx 86.98% <100%> (+0.06%) ⬆️
#tvos 86.98% <100%> (+0.06%) ⬆️
Impacted Files Coverage Δ
...urces/Extensions/SwiftStdlib/ArrayExtensions.swift 100% <100%> (ø) ⬆️
...rces/Extensions/SwiftStdlib/StringExtensions.swift 99.37% <0%> (ø) ⬆️

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 2131625...8adeb52. Read the comment docs.

Copy link
Member

@LucianoPAlmeida LucianoPAlmeida left a comment

Choose a reason for hiding this comment

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

Hey @neoneye, Thank you for your contribution :)
Great extension and implementation :)
Just suggestion about the naming, what you think about divided(by:)?
I'm not good at naming 😅 but I think divided(by:) makes sense in this context. What do you think? :)

@neoneye
Copy link
Contributor Author

neoneye commented Jan 17, 2018

I'm not a native english speaker, so I don't know.

Your name divided(by:) is great.

Ideas for other names: separate, split, order.

Copy link
Member

@LucianoPAlmeida LucianoPAlmeida left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

@neoneye
Copy link
Contributor Author

neoneye commented Jan 17, 2018

@LucianoPAlmeida I just added parameter names to the returned tuple. Perhaps this is helpful to some users. I can't think of more things. I think it's ready for merge.

@neoneye
Copy link
Contributor Author

neoneye commented Jan 18, 2018

@LucianoPAlmeida wait a little. I'm considering moving it to the Collection extension.

Copy link
Member

@LucianoPAlmeida LucianoPAlmeida left a comment

Choose a reason for hiding this comment

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

@neoneye Moving to collection is a good idea :)

@neoneye
Copy link
Contributor Author

neoneye commented Jan 18, 2018

Olà @LucianoPAlmeida

The current Array.divided(by:) in ArrayExtension has simple and short tests, easy to understand. I think the PR as it is now, is good.

I have made a collection-dividedby branch. Here the Collection.divided(by:) has unfortunately complicated the tests. I doubt anyone will ever use it for splitting dictionaries/strings. I think Arrays is the primary usecase.

Perhaps you can decide if you want divided(by:) to be placed in the ArrayExtension or in the CollectionExtension ?

@LucianoPAlmeida
Copy link
Member

@neoneye I agree that Array is the primary use case, so maybe it's enough to let it just as an Array extension :)

Copy link
Member

@SD10 SD10 left a comment

Choose a reason for hiding this comment

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

@neoneye @LucianoPAlmeida Thank you for this 💪 💯

@SD10 SD10 merged commit b7685a0 into SwifterSwift:master Jan 18, 2018
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