-
-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add Regex type #18
Add Regex type #18
Conversation
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.
The API looks good to me. Please consider my feedback and feel free to add documentation, tests and Playground usage examples to make this merge-ready. Thank you for your work!
Sources/Structs/Regex.swift
Outdated
// Created by Frederick Pietschmann on 19.03.18. | ||
// Copyright © 2018 Flinesoft. All rights reserved. | ||
// | ||
// Taken from https://github.com/sharplet/Regex, then modified to remove some weight. |
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.
Use Originally from: https://...
instead. Also place a second whitespace at the beginning to allign with the above.
Sources/Structs/Regex.swift
Outdated
/// | ||
/// - returns: `true` if the regular expression matches, otherwise `false`. | ||
public func matches(_ string: String) -> Bool { | ||
return !matches(in: string).isEmpty |
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.
I feel like this is enough reason to also introduce the firstMatch
method and use it here. This implementation will be very slow (even in average) if the underlying text is a long text. Using firstMatch
here would improve the average performance a lot for many cases. Please also note that the firstMatch(in:)
should be public as I feel like it's needed as often as matches(in:) -> [Match])
is.
Any updates on this? |
@Dschee I added the first match interface as suggested, while documentation and tests are yet to be created. |
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.
Changes look good overall. Just found a small commenting problem, probably. Other than that, only tests and docs are missing now.
Sources/Structs/Regex.swift
Outdated
/// | ||
/// - returns: An optional `Match` describing the first match, or `nil`. | ||
/// | ||
/// - note: If the match is successful, the result is also stored in `Regex.lastMatch`. |
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.
Not sure if this is true. You removed the lastMatch, no?
Nice, seems like only a new README section is missing, no? Thank you very much for all the work done so far. 👍 |
@Dschee You're welcome. With the readme now being done, this PR is now ready to merge if there aren't any issues / further suggestions. |
Looks good, merging. 🎉 |
This is my suggestion for Regex in HandySwift. Please comment on whether this is suitable.
Note: It's not tested and documented yet, I will do that on approval.
Would close #14.