Add 'removeRandomElement()' to Dictionary #497

MaxHaertwig · 2018-06-04T15:31:16Z

…to remove a random element from a dictionary

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.

SwifterSwiftBot · 2018-06-04T15:33:56Z

	4 Messages
📖	Thank you for submitting a pull request to SwifterSwift. The team will review your submission as soon as possible.
📖	iOS: Executed 491 tests, with 0 failures (0 unexpected) in 3.494 (4.166) seconds
📖	tvOS: Executed 461 tests, with 0 failures (0 unexpected) in 1.993 (2.483) seconds
📖	macOS: Executed 333 tests, with 0 failures (0 unexpected) in 1.362 (1.717) seconds

SwiftLint found issues

Warnings

File	Line	Reason
RangeReplaceableCollectionExtensions.swift	64	Lines should not have trailing whitespace.

Generated by 🚫 Danger

guykogus · 2018-06-04T15:34:09Z

Sources/Extensions/SwiftStdlib/DictionaryExtensions.swift

@@ -34,6 +34,14 @@ public extension Dictionary {
        keys.forEach { removeValue(forKey: $0) }
    }

+    /// SwifterSwift: Remove a random element from the dictionary.
+    @discardableResult public mutating func removeRandomElement() -> Value? {
+        if let key = Array(keys).randomItem {


Unfortunately you can't use SwifterSwift functions internally. The project is designed so that each function can be copy-pasted as-is into your app. The main reason is that the whole library itself can't safely be added as a dependency because of potential name clashes with other 3rd party libraries.

Okay, I will adjust the code accordingly.

codecov · 2018-06-04T15:46:47Z

Codecov Report

Merging #497 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #497      +/-   ##
==========================================
+ Coverage   93.82%   93.84%   +0.01%     
==========================================
  Files          66       66              
  Lines        2771     2778       +7     
==========================================
+ Hits         2600     2607       +7     
  Misses        171      171

Flag	Coverage Δ
#ios	`93.84% <100%> (+0.01%)`	⬆️
#osx	`54.71% <100%> (-39.12%)`	⬇️
#tvos	`88.91% <100%> (-4.92%)`	⬇️

Impacted Files	Coverage Δ
.../Extensions/SwiftStdlib/DictionaryExtensions.swift	`100% <100%> (ø)`	⬆️
...tStdlib/RangeReplaceableCollectionExtensions.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 8f853c0...7ab12a7. Read the comment docs.

SD10 · 2018-06-05T14:18:32Z

@Vyax Thanks for your pull request! This definitely feels like an odd extension though. What's your use case?

be-meyer · 2018-06-05T18:36:56Z

@SD10 I could imagin if you have something like a card game and you want to draw cards of a stack/array. But I agree, seems really odd/special.

oliviabrown9 · 2018-06-05T19:16:41Z

I’ve used a similar extension a few times in game development - I think it’s got a solid use case :)

SD10 · 2018-06-05T20:18:09Z

@oliviabrown9 @Bennx Thanks for the feedback and use cases!

I wonder if this extension could be made on Collection to be more generic?

guykogus · 2018-06-05T21:38:56Z

@SD10 this can't be made generic because keys are a specific property of Dictionary.
If we wanted, we could add an equivalent method to RangeReplaceableCollection:

extension RangeReplaceableCollection {
    @discardableResult public mutating func removeRandomElement() -> Element {
        return remove(at: index(startIndex,
                                offsetBy: numericCast(arc4random_uniform(numericCast(count)))))
    }
}

var arr = [0, 1, 2, 3, 4]
arr.removeRandomElement() // 2
arr // [0, 1, 3, 4]

LucianoPAlmeida · 2018-06-06T13:29:18Z

Hy guys
I think since this is a Dictionary extension a better naming will be removeRandomKey, to me removing a key that removing an element.
Also, that way we can implement removeRandomElement as a RandomAccessCollection extension as suggested :))

SD10 · 2018-06-06T14:54:46Z

@LucianoPAlmeida I think removeRandomKey feels like a weird name, the key is not being removed but rather the key/value pair -- ultimately the value. For example, for an array extension we may call it removeRandomElement and not removeRandomIndex

LucianoPAlmeida · 2018-06-06T15:13:52Z

@SD10 That makes sense 👍
But I'm sure the removeRandomElement is not expressive to dictionary and more to Collection/Sequence that has Collection.Element
I don't know, maybe removeRandomValue or removeRandomKeyValuePair ?

guykogus · 2018-06-06T15:18:10Z

The method of Dictionary being called in the method is removeValue(forKey:), so we could call it removeValueForRandomKey(), to be more in line with the built-in function and expressive.

guykogus · 2018-06-14T07:35:22Z

CHANGELOG.md

 - **Optional**:
  - Added `isNilOrEmpty` property to check whether an optional is nil or empty collection.
 - **UIWindow**:
  - Added `switchRootViewController` method to switch root view controller with animation. [#494](https://github.com/SwifterSwift/SwifterSwift/pull/494) by [omaralbeik](https://github.com/omaralbeik).
-
+- **Optional**:
+  - Added `isNilOrEmpty` property to check whether an optional is nil or empty collection.


You've duplicated this line

MaxHaertwig · 2018-06-19T09:19:18Z

I think this pull request should be ready for merging now.

SD10 · 2018-06-19T21:18:31Z

CHANGELOG.md

+- **UIEdgeInsets**
+  - Added `horizontal` and `vertical` properties. Also `init(inset:)` and `init(horizontal: vertical:)` initializers for convenience. [#500](https://github.com/SwifterSwift/SwifterSwift/pull/500) by [LucianoPAlmeida](https://github.com/LucianoPAlmeida).
+- **UIGestureRecognizer**:
+  - Added `removeFromView()` method to remove recognizer from the view the recognizer is attached to. [#456](https://github.com/SwifterSwift/SwifterSwift/pull/456) by [mmdock](https://github.com/mmdock).
 - **UIScrollView**:


What's up with this diff on the CHANGELOG?

I sorted the changes alphabetically.

guykogus · 2018-07-11T08:59:39Z

Tests/SwiftStdlibTests/DictionaryExtensionsTests.swift

@@ -26,6 +26,17 @@ final class DictionaryExtensionsTests: XCTestCase {
        XCTAssertFalse(dict.keys.contains("key2"))
    }

+    func testRemoveElementForRandomKey() {
+        var emptyDict = [String: String]()
+        emptyDict.removeValueForRandomKey()


Do you want to use XCTAssertNil, like you did for RangeReplaceableCollectionExtTests?

guykogus · 2018-07-11T09:00:21Z

Tests/SwiftStdlibTests/RangeReplaceableCollectionExtensionsTests.swift

+import XCTest
+@testable import SwifterSwift
+
+final class RangeReplaceableCollectionExtTests: XCTestCase {


The class name should match the file name.

I agree, but swiftlint doesn't allow class names longer than 40 characters causing the Travis build to fail.

You can hint to swiftlint to ignore a specific rule for a specific line by adding a comment above it like this:
// swiftlint:disable next identifier_name
I'm not sure if identifier_name is the exact rule that's being broken by the long class name, but it's something like that.

I believe we already have this file and class created on master, it was created in #512 if I'm remembering correctly :)

Ah true, and the class name ended up being RangeReplaceableCollectionTests, I'm guessing to avoid that issue.

SD10 · 2018-07-14T04:13:45Z

I forgot why this didn't get merged but now there's a few review conflicts + comments

…t()' to RangeReplaceableCollection

SD10 · 2018-07-28T00:08:24Z

Whats the status on this one? I see it's been updated

LucianoPAlmeida self-requested a review June 4, 2018 15:32

guykogus requested changes Jun 4, 2018

View reviewed changes

MaxHaertwig force-pushed the dictionary-remove-random-element branch from 71f7fd2 to bb47c05 Compare June 4, 2018 15:56

MaxHaertwig force-pushed the dictionary-remove-random-element branch from bb47c05 to 9ad9e48 Compare June 5, 2018 19:09

MaxHaertwig force-pushed the dictionary-remove-random-element branch from 9ad9e48 to b0d6053 Compare June 12, 2018 11:42

guykogus reviewed Jun 14, 2018

View reviewed changes

MaxHaertwig changed the title ~~Add 'removeRandomElement()' to Dictionary to remove a random element~~ Add 'removeRandomElement()' to Dictionary Jun 17, 2018

MaxHaertwig force-pushed the dictionary-remove-random-element branch 2 times, most recently from 7a586f7 to c8672b6 Compare June 18, 2018 16:46

SD10 reviewed Jun 19, 2018

View reviewed changes

MaxHaertwig force-pushed the dictionary-remove-random-element branch 3 times, most recently from e4a9b01 to f9aa957 Compare July 3, 2018 22:56

guykogus requested changes Jul 11, 2018

View reviewed changes

Add 'removeValueForRandomKey()' to Dictionary and 'removeRandomElemen…

7ab12a7

…t()' to RangeReplaceableCollection

MaxHaertwig force-pushed the dictionary-remove-random-element branch from f9aa957 to 7ab12a7 Compare July 27, 2018 15:43

guykogus approved these changes Jul 28, 2018

View reviewed changes

SD10 approved these changes Jul 28, 2018

View reviewed changes

SD10 merged commit 25a5524 into SwifterSwift:master Jul 28, 2018

MaxHaertwig deleted the dictionary-remove-random-element branch July 31, 2018 09:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'removeRandomElement()' to Dictionary #497

Add 'removeRandomElement()' to Dictionary #497

MaxHaertwig commented Jun 4, 2018 •

edited

Loading

SwifterSwiftBot commented Jun 4, 2018 •

edited

Loading

guykogus Jun 4, 2018

MaxHaertwig Jun 4, 2018

codecov bot commented Jun 4, 2018 •

edited

Loading

SD10 commented Jun 5, 2018

be-meyer commented Jun 5, 2018

oliviabrown9 commented Jun 5, 2018

SD10 commented Jun 5, 2018

guykogus commented Jun 5, 2018

LucianoPAlmeida commented Jun 6, 2018

SD10 commented Jun 6, 2018

LucianoPAlmeida commented Jun 6, 2018

guykogus commented Jun 6, 2018

guykogus Jun 14, 2018

MaxHaertwig commented Jun 19, 2018

SD10 Jun 19, 2018

MaxHaertwig Jun 22, 2018

guykogus Jul 11, 2018

guykogus Jul 11, 2018

MaxHaertwig Jul 27, 2018

guykogus Jul 27, 2018 •

edited

Loading

LucianoPAlmeida Jul 27, 2018

guykogus Jul 27, 2018

SD10 commented Jul 14, 2018

SD10 commented Jul 28, 2018

Add 'removeRandomElement()' to Dictionary #497

Add 'removeRandomElement()' to Dictionary #497

Conversation

MaxHaertwig commented Jun 4, 2018 • edited Loading

Checklist

SwifterSwiftBot commented Jun 4, 2018 • edited Loading

SwiftLint found issues

Warnings

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 4, 2018 • edited Loading

Codecov Report

SD10 commented Jun 5, 2018

be-meyer commented Jun 5, 2018

oliviabrown9 commented Jun 5, 2018

SD10 commented Jun 5, 2018

guykogus commented Jun 5, 2018

LucianoPAlmeida commented Jun 6, 2018

SD10 commented Jun 6, 2018

LucianoPAlmeida commented Jun 6, 2018

guykogus commented Jun 6, 2018

Choose a reason for hiding this comment

MaxHaertwig commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guykogus Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SD10 commented Jul 14, 2018

SD10 commented Jul 28, 2018

MaxHaertwig commented Jun 4, 2018 •

edited

Loading

SwifterSwiftBot commented Jun 4, 2018 •

edited

Loading

codecov bot commented Jun 4, 2018 •

edited

Loading

guykogus Jul 27, 2018 •

edited

Loading