Skip to content

Fix url encoder by specifying allowed characters#51

Merged
onmyway133 merged 5 commits into
masterfrom
fix/encode
Aug 16, 2017
Merged

Fix url encoder by specifying allowed characters#51
onmyway133 merged 5 commits into
masterfrom
fix/encode

Conversation

@onmyway133
Copy link
Copy Markdown
Contributor

@onmyway133 onmyway133 commented Aug 16, 2017

Copy link
Copy Markdown

@JohnSundell JohnSundell left a comment

Choose a reason for hiding this comment

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

Nice fix 👍 Some comments inline, let me know what you think 🙂

Comment thread Sources/Navigator.swift Outdated
@@ -1,4 +1,8 @@
import Foundation
#if os(OSX)
import Cocoa
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

How come this is needed, since you have only removed code from this file?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I added it by mistake. Will remove it

Comment thread Sources/Utilities.swift Outdated
@@ -0,0 +1,25 @@
import Foundation

struct Utilities {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would suggest using a different name than Utilities (which is both very ambiguous, and tends to be a "catch all" for all kinds of unrelated APIs). Since these utilities are all about encoding strings, how about implementing them in an extension on String?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ironically 😛 I just moved those from string extensions to a new class. I had some problem before when some frameworks have the same method on String

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yeah, that's a valid point. In that case, how about a StringEncoder class or something like that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I can add that

Comment thread Sources/Utilities.swift

/// Perform url encoding
///
/// - Parameter string: The source string
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Document parameter allowed

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Also I'd name that parameter allowedCharacters, since just allowed is a bit ambiguous in this context.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Also why not just pass a CharacterSet, just like to String?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right, I will add that

@onmyway133 onmyway133 merged commit 110d789 into master Aug 16, 2017
@onmyway133 onmyway133 deleted the fix/encode branch August 16, 2017 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants