Add method for first char uppercased in String #505

happiehappie · 2018-06-20T08:49:25Z

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.

guykogus · 2018-06-20T08:55:37Z

Sources/Extensions/SwiftStdlib/StringExtensions.swift

+    ///        "hello world".firstCharacterUppercased -> Optional("Hello world")
+    ///        "".firstCharacterUppercased -> nil
+    ///
+    public var firstCharacterUppercased: String? {


Why not just use capitalized, or better yet localizedCapitalized?

Because capitalize will uppercase first characters in every word, not practical when your localized strings are in the form of a sentence message

not using localizedCapitalized in this code is simply because localizedCapitalized's deployment target is 9.0

SwifterSwiftBot · 2018-06-20T09:03:16Z

	4 Messages
📖	Thank you for submitting a pull request to SwifterSwift. The team will review your submission as soon as possible.
📖	iOS: Executed 495 tests, with 0 failures (0 unexpected) in 3.368 (3.813) seconds
📖	tvOS: Executed 465 tests, with 0 failures (0 unexpected) in 2.067 (2.501) seconds
📖	macOS: Executed 334 tests, with 0 failures (0 unexpected) in 1.308 (1.647) seconds

Generated by 🚫 Danger

happiehappie · 2018-06-20T09:22:17Z

Is the unit test for SwifterSwiftTests passed in current master? I didn't change that file though o.O

guykogus · 2018-06-20T09:06:05Z

Sources/Extensions/SwiftStdlib/StringExtensions.swift

+    ///        "hello world".firstCharacterUppercased -> Optional("Hello world")
+    ///        "".firstCharacterUppercased -> nil
+    ///
+    public var firstCharacterUppercased: String? {


I don't think this should return nil for an empty string, it should just return the empty string. That would be more consistent with the other methods. E.g. "".uppercased() returns "" not nil.

Shall I change the return type to String than an optional string then?

Don't forget to update your tests, too.

…to be non-optional

codecov · 2018-06-20T09:31:13Z

Codecov Report

Merging #505 into master will increase coverage by 2.17%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #505      +/-   ##
==========================================
+ Coverage    91.7%   93.87%   +2.17%     
==========================================
  Files          64       66       +2     
  Lines        2760     2791      +31     
==========================================
+ Hits         2531     2620      +89     
+ Misses        229      171      -58

Flag	Coverage Δ
#ios	`93.87% <100%> (+2.17%)`	⬆️
#osx	`88.96% <100%> (+2.62%)`	⬆️
#tvos	`88.96% <100%> (+2.62%)`	⬆️

Impacted Files	Coverage Δ
...rces/Extensions/SwiftStdlib/StringExtensions.swift	`100% <100%> (ø)`	⬆️
.../Extensions/SwiftStdlib/DictionaryExtensions.swift	`100% <0%> (ø)`	⬆️
...urces/Extensions/SwiftStdlib/ArrayExtensions.swift	`100% <0%> (ø)`	⬆️
...tStdlib/RangeReplaceableCollectionExtensions.swift	`100% <0%> (ø)`
...SwiftStdlib/RandomAccessCollectionExtensions.swift	`100% <0%> (ø)`
Sources/Extensions/Shared/ColorExtensions.swift	`98.05% <0%> (+0.97%)`	⬆️
Sources/Extensions/UIKit/UIImageExtensions.swift	`97.82% <0%> (+2.27%)`	⬆️
.../Extensions/UIKit/UIViewControllerExtensions.swift	`91.89% <0%> (+3%)`	⬆️
.../Extensions/UIKit/UICollectionViewExtensions.swift	`77.55% <0%> (+4.08%)`	⬆️
... and 6 more

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 a695f4c...4896f87. Read the comment docs.

guykogus · 2018-06-20T09:33:25Z

Tests/SwiftStdlibTests/StringExtensionsTests.swift

@@ -50,6 +50,11 @@ final class StringExtensionsTests: XCTestCase {
        XCTAssertEqual("Hello".firstCharacterAsString, "H")
    }

+    func testFirstCharacterUppercased() {
+        XCTAssertNotNil("hello world".firstCharacterUppercased)


This isn't a useful test, since the return value of the method isn't optional. Better to test the empty string.

guykogus · 2018-06-20T09:35:11Z

Sources/Extensions/SwiftStdlib/StringExtensions.swift

+    ///        "".firstCharacterUppercased -> ""
+    ///
+    public var firstCharacterUppercased: String {
+        guard let first = first else { return "" }


Use return self, not return "". I know that it's most likely to be the same, but you may catch some edge-cases where there's an unfinished unicode character or something.

guykogus · 2018-06-20T09:41:28Z

Tests/SwiftStdlibTests/StringExtensionsTests.swift

@@ -51,7 +51,7 @@ final class StringExtensionsTests: XCTestCase {
    }

    func testFirstCharacterUppercased() {
-        XCTAssertNotNil("hello world".firstCharacterUppercased)
+        XCTAssertNotNil("".firstCharacterUppercased)


Again, checking that it's not nil is always going to be true. I meant more like XCTAssertEqual("".firstCharacterUppercased, "")

Damn lol I totally misunderstood you, my bad.

SD10 · 2018-06-22T04:04:04Z

Sources/Extensions/SwiftStdlib/StringExtensions.swift

+    ///        "hello world".firstCharacterUppercased -> "Hello world"
+    ///        "".firstCharacterUppercased -> ""
+    ///
+    public var firstCharacterUppercased: String {


Actually, I think we should make this a method and not a property. Most String operations in Swift are methods because they're not constant time operations

capitalized is a property though, but then again uppercased is () :x

Let's follow the same pattern as uppercased @happiehappie

Agreed.

A copy of a string with its first letter uppercased isn't a property of the string, it's a transformation, so semantically it follows that it's a function.

I believe we have an agreed standard that anything that isn't constant time is a function, not a property.

SD10

See comments

happiehappie

Thoughts?

happiehappie · 2018-06-22T04:27:52Z

Sources/Extensions/SwiftStdlib/StringExtensions.swift

+    ///        "hello world".firstCharacterUppercased -> "Hello world"
+    ///        "".firstCharacterUppercased -> ""
+    ///
+    public var firstCharacterUppercased: String {


capitalized is a property though, but then again uppercased is () :x

guykogus · 2018-07-11T08:54:58Z

@happiehappie still think this should be a func, not a var.

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

Any update on this one @happiehappie?

happiehappie · 2018-07-22T10:17:31Z

I'll push in coming days, was travelling~

marcocapano · 2018-08-01T20:45:50Z

@happiehappie still willing to make the pull request?

LucianoPAlmeida · 2018-08-03T02:36:49Z

Hey @happiehappie :)) Can you update this branch with master?

LucianoPAlmeida · 2018-08-03T02:40:42Z

Sources/Extensions/SwiftStdlib/StringExtensions.swift

@@ -105,7 +105,7 @@ public extension String {
 		return String(first)
 	}

-	#if canImport(Foundation)
+    #if canImport(Foundation)


Do we really need this check here? 🤔 I mean ... String is a type from the stdlib and to me since there's no bridging to NSString or usage of any foundation method it seems like there's no need for that :))

String actually has some extensions in Foundation, e.g. when using Data, which is part of Foundation, not the stdlib. Still, we should only be using this check for those situations where we use Foundation functions/classes.

Hmmmm. Guess that doesn't hurt

SD10 · 2018-08-11T16:40:53Z

Thank you for contributing to SwifterSwift! I've invited you to join the SwifterSwift GitHub organization - no pressure to accept! If you'd like more information on what that means, check out our contributing guidelines. Feel free to reach out if you have any questions! 😃

Stanley Lim added 4 commits June 20, 2018 16:35

Add method that returns a string with first character uppercased

fb89593

update Changelog

3dc79c9

Added comments

9f035d7

Update spacing for consistency

7891b08

happiehappie changed the title ~~Add~~ Add method for first char uppercased in String Jun 20, 2018

Stanley Lim added 2 commits June 20, 2018 16:50

Update changelog #

da2d70e

Added unit test

e7c0bc0

guykogus reviewed Jun 20, 2018

View reviewed changes

guykogus requested changes Jun 20, 2018

View reviewed changes

Complied to Danger/Swiftlint for whitespace, update return parameter …

ab1ca32

…to be non-optional

Update comment example

63e5cad

guykogus requested changes Jun 20, 2018

View reviewed changes

Update test case and return value

d5493d3

guykogus reviewed Jun 20, 2018

View reviewed changes

Update test case

9fb4d46

guykogus previously approved these changes Jun 20, 2018

View reviewed changes

SD10 approved these changes Jun 22, 2018

View reviewed changes

SD10 reviewed Jun 22, 2018

View reviewed changes

SD10 requested changes Jun 22, 2018

View reviewed changes

happiehappie commented Jun 24, 2018

View reviewed changes

marcocapano previously approved these changes Aug 1, 2018

View reviewed changes

update extension var to be a method

4896f87

happiehappie dismissed marcocapano’s stale review via 4896f87 August 2, 2018 15:50

happiehappie dismissed guykogus’s stale review via 4896f87 August 2, 2018 15:50

LucianoPAlmeida reviewed Aug 3, 2018

View reviewed changes

LucianoPAlmeida added the new extension label Aug 9, 2018

guykogus approved these changes Aug 11, 2018

View reviewed changes

SD10 approved these changes Aug 11, 2018

View reviewed changes

SD10 merged commit 58be3ee into SwifterSwift:master Aug 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add method for first char uppercased in String #505

Add method for first char uppercased in String #505

happiehappie commented Jun 20, 2018 •

edited

Loading

guykogus Jun 20, 2018

happiehappie Jun 20, 2018

happiehappie Jun 20, 2018

SwifterSwiftBot commented Jun 20, 2018 •

edited

Loading

happiehappie commented Jun 20, 2018

guykogus Jun 20, 2018

happiehappie Jun 20, 2018

guykogus Jun 20, 2018

guykogus Jun 20, 2018

codecov bot commented Jun 20, 2018 •

edited

Loading

guykogus Jun 20, 2018

guykogus Jun 20, 2018

guykogus Jun 20, 2018

happiehappie Jun 20, 2018

SD10 Jun 22, 2018

happiehappie Jun 22, 2018

SD10 Jun 24, 2018

guykogus Jun 25, 2018

SD10 left a comment

happiehappie left a comment

happiehappie Jun 22, 2018

guykogus commented Jul 11, 2018

SD10 commented Jul 14, 2018

happiehappie commented Jul 22, 2018

marcocapano commented Aug 1, 2018

LucianoPAlmeida commented Aug 3, 2018

LucianoPAlmeida Aug 3, 2018

guykogus Aug 11, 2018

SD10 Aug 11, 2018

SD10 commented Aug 11, 2018

Add method for first char uppercased in String #505

Add method for first char uppercased in String #505

Conversation

happiehappie commented Jun 20, 2018 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SwifterSwiftBot commented Jun 20, 2018 • edited Loading

happiehappie commented Jun 20, 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

codecov bot commented Jun 20, 2018 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SD10 left a comment

Choose a reason for hiding this comment

happiehappie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guykogus commented Jul 11, 2018

SD10 commented Jul 14, 2018

happiehappie commented Jul 22, 2018

marcocapano commented Aug 1, 2018

LucianoPAlmeida commented Aug 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SD10 commented Aug 11, 2018

happiehappie commented Jun 20, 2018 •

edited

Loading

SwifterSwiftBot commented Jun 20, 2018 •

edited

Loading

codecov bot commented Jun 20, 2018 •

edited

Loading