Skip to content
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

use struct instead of class, overloading operators #5

Merged
merged 1 commit into from Jan 22, 2016

Conversation

nmn
Copy link
Contributor

@nmn nmn commented Jan 6, 2016

  1. Updated Class to be a Struct instead. This makes the whole thing immutable, and get value semantics. A regular expression is clearly 'data' and should have value semantics.
  2. Added two more functions for the custom operators, which just allow you to reverse the arguments.
    if a =~ b is true, b =~ a should also be true. This can be done because string matching is commutative.

@nmn
Copy link
Contributor Author

nmn commented Jan 6, 2016

Note: as far as I can tell, the current version doesn't allow you to use VerEx in a .then method, but it should. I'll think about it and fix it when possible.

@nmn nmn mentioned this pull request Jan 7, 2016
2 tasks
@nmn
Copy link
Contributor Author

nmn commented Jan 18, 2016

Ping! @jehna

@@ -6,13 +6,15 @@
// Copyright (c) 2014 Dominique d'Argent. All rights reserved.
//

//: Playground - noun: a place where people can play
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, was using a playground to type and test it out. Will get rid of it in a new commit.

@jehna
Copy link
Contributor

jehna commented Jan 19, 2016

@nmn seems that there are some tests that are failing. Can you check those out?

screen shot 2016-01-19 at 09 46 55

@nmn
Copy link
Contributor Author

nmn commented Jan 19, 2016

Sure thing. Get back to you with updates soon

@nmn
Copy link
Contributor Author

nmn commented Jan 19, 2016

The tests should be fixed now. But I'll confirm later today.

public func startOfLine(enabled enabled: Bool = true) -> VerbalExpressions {
return VerbalExpressions(
prefixes: enabled ? "^" : "",
source: self.source,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think self. is discouraged when not necessarily needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not seen it mentioned either way. self. just feels a little more explicit to me. I don't have any strong feeling about that however, I'll clean them up.

@nubbel
Copy link
Contributor

nubbel commented Jan 19, 2016

Thanks for working on that!
I've posted my comments inline.

@nmn
Copy link
Contributor Author

nmn commented Jan 20, 2016

Instead of going with full fledged Lenses, I've defined an overloaded set method that lets you change one property at a time. I've then used that method in all other methods to reduce boilerplate. I think this works better than Lenses would.

Also, I've gotten rid of self. from the code.

@Danappelxx
Copy link

Good call - I think that's a better approach.

@nubbel
Copy link
Contributor

nubbel commented Jan 20, 2016

@nmn Great idea, much simpler!

Final nitpicks, then I'm happy to merge:

  1. The set function should be private. There's no need to expose that to clients, given that the variables it sets are also private.
  2. One set function should be enough. You could make use of optionals.

@nubbel
Copy link
Contributor

nubbel commented Jan 20, 2016

Regarding my second point, I was thinking about something like this:

public struct A {
    private let x: String
    private let y: String
    private let z: String
}

private extension A {
    func set(x x: String? = nil, y: String? = nil, z: String? = nil) -> A {
        return A(
            x: x ?? self.x,
            y: y ?? self.y,
            z: z ?? self.z
        )
    }
}

let a = A(x: "Hello", y: "world", z: "!")
String(a) // => "A(x: "Hello", y: "world", z: "!")"
let b = a.set(y: "nubbel")
String(b) // => "A(x: "Hello", y: "nubbel", z: "!")"
let c = b.set(x: "Bonjour")
String(c) // => "A(x: "Bonjour", y: "nubbel", z: "!")"

What would you say?

@Danappelxx
Copy link

@nubbel that's a better approach - can't believe I didn't spot it myself.

@nmn
Copy link
Contributor Author

nmn commented Jan 21, 2016

@nubbel that looks great. Didn't know I could combine optionals and default values like that. I'll make the update.

return try! NSRegularExpression(pattern: pattern, options: options)
}

func set(prefixes prefixes: String? = nil, source: String? = nil, suffixes: String? = nil, suffixes: NSRegularExpressionOptions? = nil) -> A {
Copy link
Contributor

Choose a reason for hiding this comment

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

A should probably be VerbalExpressions ;)

@nubbel
Copy link
Contributor

nubbel commented Jan 22, 2016

Looks good to me now!
Kindly squash the commits into one single commit for merge. Thanks!

@nmn
Copy link
Contributor Author

nmn commented Jan 22, 2016

DONE

nubbel added a commit that referenced this pull request Jan 22, 2016
use struct instead of class, overloading operators
@nubbel nubbel merged commit e53e02d into VerbalExpressions:master Jan 22, 2016
@nubbel
Copy link
Contributor

nubbel commented Jan 22, 2016

Thanks!

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.

None yet

4 participants