-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix url encoder by specifying allowed characters #51
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
import Foundation | ||
|
||
struct Utilities { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would suggest using a different name than There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that's a valid point. In that case, how about a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I can add that |
||
|
||
/// Perform url encoding | ||
/// | ||
/// - Parameter string: The source string | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Document parameter There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also I'd name that parameter There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also why not just pass a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, I will add that |
||
/// - Returns: The encoded string | ||
static func encode(string: String, allowed: String) -> String { | ||
var set = CharacterSet.urlPathAllowed | ||
allowed.unicodeScalars.forEach { | ||
set.insert($0) | ||
} | ||
|
||
return string.addingPercentEncoding(withAllowedCharacters: set) ?? string | ||
} | ||
|
||
/// Perform url decoding | ||
/// | ||
/// - Parameter string: The encoded string | ||
/// - Returns: The percented decoded string | ||
static func decode(string: String) -> String { | ||
return string.removingPercentEncoding ?? string | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
import Foundation | ||
import XCTest | ||
@testable import Compass | ||
|
||
class UtilitiesTests: XCTestCase { | ||
func testEncode() { | ||
let urn = "organization:hyper oslo:simply awesome" | ||
let encodedUrn = Utilities.encode(string: urn, allowed: ":") | ||
|
||
XCTAssertEqual(encodedUrn, "organization:hyper%20oslo:simply%20awesome") | ||
} | ||
|
||
func testDecode() { | ||
let urn = "organization:hyper oslo:simply awesome" | ||
let encodedUrn = Utilities.encode(string: urn, allowed: ":") | ||
|
||
XCTAssertEqual(Utilities.decode(string: encodedUrn), urn) | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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