Removing deprecated string.characters calls and xcode 9.1 recommend settings. #305

LucianoPAlmeida · 2017-11-05T12:19:07Z

Xcode 9.1 project recommended settings
Removing deprecated string.characters calls.

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.

Removing deprecated string.characters calls.

SwifterSwiftBot · 2017-11-05T12:23:19Z

	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

SD10 · 2017-11-05T12:33:06Z

Sources/Extensions/Shared/ColorExtensions.swift

@@ -92,7 +92,7 @@ public extension Color {
 	/// SwifterSwift: Short hexadecimal value string (read-only, if applicable).
 	public var shortHexString: String? {
 		let string = hexString.replacingOccurrences(of: "#", with: "")
-		let chrs = Array(string.characters)
+		let chrs = Array(string)


Won't this fail now?

I think not, it returns an array of characters the same way as before :)

SD10 · 2017-11-05T12:35:28Z

Sources/Extensions/SwiftStdlib/StringExtensions.swift

@@ -39,7 +39,7 @@ public extension String {
 	/// SwifterSwift: Array of characters of a string.
 	///
 	public var charactersArray: [Character] {
-		return Array(characters)
+		return Array(self)


Do we get an array of characters here or an array of a Single string?

Guess I have to read about the new Strings 🤔 Implicit cast?

It returns an array of characters :)

I think is because string now has collection conformance again, so it uses Array(s: Sequence) and returns a sequence of its characters. Did this test on playground

Can we do this? Forgive me I haven't had to deal with this yet 😃 But we may want to re-evaluate all the string extensions in general and see if some are obsolete

var chars: [Characters] = "Swift"

I think not, swift compiler complains about cannot convert String to [Character],

@LucianoPAlmeida Appreciate you testing that! It's very hard for me to run a playground on my slow iMac 😭

codecov · 2017-11-05T12:38:31Z

Codecov Report

Merging #305 into master will decrease coverage by 0.22%.
The diff coverage is 74.24%.

@@            Coverage Diff             @@
##           master     #305      +/-   ##
==========================================
- Coverage   94.66%   94.43%   -0.23%     
==========================================
  Files         103      104       +1     
  Lines        5994     5988       -6     
==========================================
- Hits         5674     5655      -19     
- Misses        320      333      +13

Flag	Coverage Δ
#ios	`94.24% <74.24%> (-0.23%)`	⬇️
#osx	`62.47% <68.18%> (-0.31%)`	⬇️
#tvos	`89.57% <74.24%> (-0.29%)`	⬇️

Impacted Files	Coverage Δ
...SwiftStdlib/Deprecated/SwiftStdlibDeprecated.swift	`0% <0%> (ø)`
...rces/Extensions/SwiftStdlib/StringExtensions.swift	`99.39% <100%> (-0.02%)`	⬇️
Tests/SwiftStdlibTests/StringExtensionsTests.swift	`98.07% <100%> (-0.05%)`	⬇️
Tests/SwiftStdlibTests/ArrayExtensionsTests.swift	`98.29% <100%> (+0.34%)`	⬆️
Sources/Extensions/Shared/ColorExtensions.swift	`98.95% <100%> (ø)`	⬆️
...s/Extensions/SwiftStdlib/CharacterExtensions.swift	`100% <100%> (ø)`	⬆️
Tests/UIKitTests/UISliderExtensionsTests.swift	`100% <0%> (+9.37%)`	⬆️

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 e92a82e...1a71515. Read the comment docs.

SD10 · 2017-11-05T13:37:41Z

Sources/Extensions/SwiftStdlib/StringExtensions.swift

@@ -229,7 +229,7 @@ public extension String {
 	///		"Hello world!".length -> 12
 	///
 	public var length: Int {


We should probably deprecate this one. It's not even a good name because it can easily be confused with NSString.length

💯, also String.count can be used in Swift 4

SD10 · 2017-11-05T13:40:21Z

Sources/Extensions/SwiftStdlib/StringExtensions.swift

@@ -401,7 +401,8 @@ public extension String {
 	///
 	/// - Returns: The reversed string.
 	public func reversed() -> String {


We should remove this one too. We can just use String(str.reversed()) if we even need String typing

SD10 · 2017-11-05T13:42:34Z

Sources/Extensions/SwiftStdlib/StringExtensions.swift

@@ -575,7 +576,7 @@ public extension String {
 	/// - Parameter string: substring to search for.
 	/// - Returns: first index of substring in string (if applicable).
 	public func firstIndex(of string: String) -> Int? {


This is probably obsolete now that String is a Collection?
We can use func index(of element: Self.Element) -> Self.Index? I think?

SD10 · 2017-11-05T13:46:07Z

Sources/Extensions/SwiftStdlib/StringExtensions.swift

@@ -620,10 +621,10 @@ public extension String {
 	///   - length: amount of characters to be sliced after given index.
 	/// - Returns: sliced substring of length number of characters (if applicable) (example: "Hello World".slicing(from: 6, length: 5) -> "World")
 	public func slicing(from i: Int, length: Int) -> String? {


Now we can subscript because it's a collection so this can probably go too 😅

I don't know about this one. I think handles some safe convenience cases that maybe aren't done with the subscript. @SD10 @omaralbeik What you think?

I think we should turn it into a safe subscript that accepts a Range<String.Index> and returns a Substring? then. The name of this method doesn't make sense, I'd expect to get an ArraySlice out of it. Since it has ing it also makes me think it's mutating and it's not 😅

I don't know if it can be turned in a subscript with range because the second parameter is a length, not an end index 😅 I think worth just rename and returning a substring instead. @SD10 @omaralbeik What you think?

That's a good point @LucianoPAlmeida 🤔 But I'm having trouble thinking of a place where someone would be using a length and not an endIndex considering they have a startIndex. You can always get the endIndex by incrementing the startIndex by n

I agree. So you think will be better deprecating this?

Yeah, we can always replace it with the subscript version in another PR. Sorry to make so many changes. I should've thought about this before we even released SwifterSwift 4.0. It completely slipped my mind

What about the mutating version? Should be maintained?

@LucianoPAlmeida lets not make this change right now... I just opened the file and there are a ton of these slice methods. We also already have subscripts from ranges but they return String? and not Substring? It's too much for me to look at right now...

@SD10 No problem we can open a issue/PR later with only this methods to discuss :)

SD10 · 2017-11-05T13:46:40Z

Sources/Extensions/SwiftStdlib/StringExtensions.swift

@@ -684,10 +685,10 @@ public extension String {
 	/// - Parameter i: string index the slicing should start from.
 	/// - Returns: sliced substring starting from start index (if applicable) (example: "Hello world".slicing(at: 6) -> "world")
 	public func slicing(at i: Int) -> String? {


SD10 · 2017-11-05T13:50:58Z

Sources/Extensions/SwiftStdlib/StringExtensions.swift

@@ -710,7 +711,7 @@ public extension String {
 	/// - Parameter separator: separator to split string by.
 	/// - Returns: array of strings separated by given string.
 	public func splitted(by separator: Character) -> [String] {


Not sure about this one but it looks like split(separator: Element) is the way to go

SD10

@LucianoPAlmeida Let me know what you think of the methods I've commented. I think a lot of them have become obsolete because they were simply added as convenience to not have to reference the underlying characters Array. We get a lot of this out of the box form Collection now

LucianoPAlmeida · 2017-11-05T14:35:04Z

@SD10 sure, I'll take a look at them as soon as possible :)

_{Sent with GitHawk}

omaralbeik

Thank you @LucianoPAlmeida
I think we need to check all String extensions as @SD10 mentioned

omaralbeik · 2017-11-05T16:58:18Z

Sources/Extensions/SwiftStdlib/StringExtensions.swift

@@ -86,7 +86,7 @@ public extension String {
 	///		"".firstCharacterAsString -> nil
 	///
 	public var firstCharacterAsString: String? {
-		guard let first = characters.first else {
+		guard let first = self.first else {


We might want to rename first to firstCharacter to avoid variable shadowing

I think we can't rename first because is a native String property now that its conformance with Collection was restored.

@LucianoPAlmeida I meant guard let firstCharacter = self.first else { ...

omaralbeik · 2017-11-05T16:58:34Z

Sources/Extensions/SwiftStdlib/StringExtensions.swift

@@ -210,7 +210,7 @@ public extension String {
 	///		"".lastCharacterAsString -> nil
 	///
 	public var lastCharacterAsString: String? {
-		guard let last = characters.last else {
+		guard let last = self.last else {


We might want to rename last to lastCharacter to avoid variable shadowing

Same thing like first above :)

We should probably just remove these as well 😅 "Some string".last and casting when we need it should be prefered

omaralbeik · 2017-11-05T17:02:31Z

Sources/Extensions/SwiftStdlib/StringExtensions.swift

@@ -386,7 +386,7 @@ public extension String {
 	///
 	/// - Returns: The most common character.
 	public func mostCommonCharacter() -> String {


how come mostCommonCharacter is returning String instead of Character here 🤔

omaralbeik · 2017-11-05T17:03:35Z

Sources/Extensions/SwiftStdlib/StringExtensions.swift

@@ -442,7 +443,7 @@ public extension String {
 	///
 	/// - Parameter i: index.
 	public subscript(safe i: Int) -> String? {


Shouldn't this return Character? as well?

LucianoPAlmeida · 2017-11-05T20:09:08Z

@omaralbeik @SD10 Working on those that should be deprecated :) How deprecation should be done? Just annotate the method? Annotate and move to another file? In case of renaming, should I remove the previous method?

omaralbeik · 2017-11-06T08:32:25Z

@LucianoPAlmeida Here is an example from v3.2

basically:
1- create a new directory inside SwiftStdlib called Deprecated
2- create a new SwiftStdlibDeprecated.swift file
3- move methods to deprecate to that file
4- mark them as deprecated
5- done!

…nd clean up the tests. Adding changeling entries for the deprecations.

omaralbeik · 2017-11-11T21:49:30Z

@SD10 @LucianoPAlmeida Please let me know if we can merge this soon, as other PRs are depending on it. We'll probably look again on String extensions later

LucianoPAlmeida · 2017-11-11T22:08:58Z

Hey @omaralbeik That's it for this one, I was just waiting for both of you guys review it :)

SD10 · 2017-11-12T15:17:50Z

CHANGELOG.md

+    - `slicing(from: to:)` is deprecated, use `string[safe: start..<end]`
+    - `firstIndex(of:)` is deprecated, use the natives `index(of: Character)` or `range(of: StringProtocol)` instead.
+    - `splitted(by:)` is deprecated,  use the native `split(separator: )` instead.
+    - `reversed() -> String` is deprecated, use the Swift 4 new `reversed() -> ReversedCollection<String>` 


@LucianoPAlmeida Please add the PR number as a reference under this part of the CHANGELOG

SD10

I'm good with this 👍

omaralbeik

Thank you guys!

Xcode 9.1 project recommended settings

ee74bd7

Removing deprecated string.characters calls.

LucianoPAlmeida requested a review from omaralbeik November 5, 2017 12:19

SD10 reviewed Nov 5, 2017

View reviewed changes

LucianoPAlmeida requested a review from SD10 November 5, 2017 13:29

SD10 reviewed Nov 5, 2017

View reviewed changes

omaralbeik reviewed Nov 5, 2017

View reviewed changes

Merge remote-tracking branch 'origin/master'

c4a1ef6

LucianoPAlmeida added 4 commits November 7, 2017 21:00

Deprecating some string extensions, adding them to a separated file a…

bf722ee

…nd clean up the tests. Adding changeling entries for the deprecations.

Merge remote-tracking branch 'origin/master'

0a728eb

Changing deprecation version :)

5dffbdc

kaphacius mentioned this pull request Nov 11, 2017

#312 Update travis to use xcode 9.1 #314

Merged

1 task

Merge remote-tracking branch 'origin/master'

2aaac38

Changes on tests array.

6ff0fe4

SD10 reviewed Nov 12, 2017

View reviewed changes

SD10 approved these changes Nov 12, 2017

View reviewed changes

Adding PR number on changelog :)

1a71515

omaralbeik approved these changes Nov 12, 2017

View reviewed changes

omaralbeik merged commit 50303ce into SwifterSwift:master Nov 12, 2017

Removing deprecated string.characters calls and xcode 9.1 recommend settings. #305

Removing deprecated string.characters calls and xcode 9.1 recommend settings. #305

Conversation

LucianoPAlmeida commented Nov 5, 2017

Checklist

SwifterSwiftBot commented Nov 5, 2017 • edited Loading

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

Choose a reason for hiding this comment

codecov bot commented Nov 5, 2017 • 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

Choose a reason for hiding this comment

LucianoPAlmeida Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucianoPAlmeida Nov 7, 2017 • edited Loading

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

LucianoPAlmeida commented Nov 5, 2017

omaralbeik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omaralbeik Nov 11, 2017 • edited Loading

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

LucianoPAlmeida commented Nov 5, 2017

omaralbeik commented Nov 6, 2017

omaralbeik commented Nov 11, 2017

LucianoPAlmeida commented Nov 11, 2017

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

omaralbeik left a comment

Choose a reason for hiding this comment

SwifterSwiftBot commented Nov 5, 2017 •

edited

Loading

codecov bot commented Nov 5, 2017 •

edited

Loading

LucianoPAlmeida Nov 7, 2017 •

edited

Loading

LucianoPAlmeida Nov 7, 2017 •

edited

Loading

omaralbeik Nov 11, 2017 •

edited

Loading