Added function for changing a query parameter in an URL #60

nharbo · 2019-01-09T15:15:49Z

No description provided.

merging master into develop

siemensikkema

Looks handy! I made some suggestions that are hopefully a bit helpful...

siemensikkema · 2019-01-09T16:00:20Z

Codemine.xcodeproj/xcshareddata/xcschemes/Codemine.xcscheme

-      shouldUseLaunchSchemeArgsEnv = "YES"
-      codeCoverageEnabled = "NO">
+      codeCoverageEnabled = "YES"
+      shouldUseLaunchSchemeArgsEnv = "YES">


Are these changes by accident?

I showed him how to enable code coverage. One could argue if it should be part of the entire framework or just set up locally

siemensikkema · 2019-01-09T16:04:32Z

Codemine/Extensions/NSURL+Utilities.swift

+     - withName: The `String` representation of the name of the queryParameter you want to change the value of
+     - toValue: The `String` representation of the new value for the queryParameter
+     - returns: a new `URL` instance with the changed queryParameters or nil if the change failed
+     */


tip: Xcode can help creating the doc section for a function. Editor > Structure > Add Documentation. Or Command Option /

It saves time and helps create a standardized format for docs.

siemensikkema · 2019-01-09T16:05:39Z

Codemine/Extensions/NSURL+Utilities.swift

+     - toValue: The `String` representation of the new value for the queryParameter
+     - returns: a new `URL` instance with the changed queryParameters or nil if the change failed
+     */
+    public func changeQueryParamValue(for url: URL, withName: String, toValue: String) -> URL? {


you can change the internal names for parameters like this:

public func changeQueryParamValue(for url: URL, withName name: String, toValue value: String) -> URL?

In the function withName and toValue don't make as much sense.

Have you considered changing the semantics from "change if it's there" to "add or replace"?

why are you passing in the url parameter? You are already working on an URL right?

Here is an example from your test

let url = URL(string: "http://example.com?token=addtokenhere") let queryParamUrl = url.changeQueryParamValue(for: url, withName: "token", toValue: "usertoken")

I would expect that I could call it like so instead:

let queryParamUrl = url.changeQueryParamValue(for: "token", to: "usertoken")

siemensikkema · 2019-01-09T16:31:47Z

Codemine/Extensions/NSURL+Utilities.swift

+            if
+                var component = URLComponents(url: url, resolvingAgainstBaseURL: false),
+                var queryItems = component.queryItems,
+                var firstParam = component.queryItems?.filter({$0.name == withName}).first,


How about: let index = queryItems.firstIndex(where: {$0.name == withName}) and then var queryItem = queryItems[index]
That uses the just unwrapped queryItems and doesn't go through the whole list before returning only the first item and it traverses the array only once (instead of first finding the query item and then finding the index of that query item).

pbodsk · 2019-01-10T07:50:59Z

Codemine/Extensions/NSURL+Utilities.swift

+     - toValue: The `String` representation of the new value for the queryParameter
+     - returns: a new `URL` instance with the changed queryParameters or nil if the change failed
+     */
+    public func changeQueryParamValue(for url: URL, withName: String, toValue: String) -> URL? {


why are you passing in the url parameter? You are already working on an URL right?

Here is an example from your test

let url = URL(string: "http://example.com?token=addtokenhere") let queryParamUrl = url.changeQueryParamValue(for: url, withName: "token", toValue: "usertoken")

I would expect that I could call it like so instead:

let queryParamUrl = url.changeQueryParamValue(for: "token", to: "usertoken")

pbodsk · 2019-01-10T07:51:43Z

Codemine.xcodeproj/xcshareddata/xcschemes/Codemine.xcscheme

-      shouldUseLaunchSchemeArgsEnv = "YES"
-      codeCoverageEnabled = "NO">
+      codeCoverageEnabled = "YES"
+      shouldUseLaunchSchemeArgsEnv = "YES">


I showed him how to enable code coverage. One could argue if it should be part of the entire framework or just set up locally

pbodsk · 2019-01-10T07:52:46Z

Codemine/Extensions/NSURL+Utilities.swift

+     - returns: a new `URL` instance with the changed queryParameters or nil if the change failed
+     */
+    public func changeQueryParamValue(for url: URL, withName: String, toValue: String) -> URL? {
+        var internalUrl = url


here you could just say var internalURL = self...you are adding this extension to the URL

pbodsk · 2019-01-10T07:56:00Z

Codemine/Extensions/NSURL+Utilities.swift

+     */
+    public func changeQueryParamValue(for url: URL, withName: String, toValue: String) -> URL? {
+        var internalUrl = url
+        if let _ = internalUrl.value(forParameter: withName) {


how about checking against nil instead?

if internalUrl.value(forParameter: name) != nil {

siemensikkema

Looks good!

pbodsk

Looks fine, nothing further

🚀

Added Table of contents Added the source code to the README Added missing usage examples Added Description of functionality for each section

Testing approach for linking to section with emoji

Removing emojis from section headers because they break the linking in the table of contents

Updated REAME.md

fixes tests

Feature/migrate to swift 4

nharbo and others added 2 commits January 9, 2019 15:31

Merge pull request #59 from nodes-ios/master

75fdcd3

merging master into develop

function for changing queryparam + test with 100% coverage added

9b1bcba

siemensikkema reviewed Jan 9, 2019

View reviewed changes

pbodsk reviewed Jan 10, 2019

View reviewed changes

changes applied

ea0f85d

siemensikkema approved these changes Jan 18, 2019

View reviewed changes

nharbo requested a review from pbodsk January 18, 2019 10:14

fixes applied

62ba6d1

pbodsk reviewed Jan 18, 2019

View reviewed changes

BobDeKort and others added 7 commits April 15, 2019 16:06

Updated REAME.md

ac0e6fb

Added Table of contents Added the source code to the README Added missing usage examples Added Description of functionality for each section

Update README.md

872109b

Testing approach for linking to section with emoji

Removed emojis from section header

6db7e62

Removing emojis from section headers because they break the linking in the table of contents

Merge pull request #61 from nodes-ios/readme_with_code

ce542ca

Updated REAME.md

migrates to Swift 4.2

e1443fb

fixes tests

Merge pull request #63 from nodes-ios/feature/migrate-to-swift-4

01996b9

Feature/migrate to swift 4

fixing warnings

714757c

pbodsk merged commit e7299a1 into master May 3, 2019

Added function for changing a query parameter in an URL #60

Added function for changing a query parameter in an URL #60

Uh oh!

Conversation

nharbo commented Jan 9, 2019

Uh oh!

siemensikkema left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siemensikkema left a comment

Choose a reason for hiding this comment

Uh oh!

pbodsk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants