Nimble icon indicating copy to clipboard operation
Nimble copied to clipboard

Protocol Extension Discoverability

Open stephencelis opened this issue 9 years ago • 23 comments

Would there be any interest in updating Nimble's matchers to work using protocol extensions for discoverability?

E.g.,

// before
expect(1 + 1).to(equal(2))
expect(1.2).to(beCloseTo(1.1, within: 0.1))
expect(3) > 2
expect("seahorse").to(contain("sea"))
expect(["Atlantic", "Pacific"]).toNot(contain("Mississippi"))
expect(ocean.isClean).toEventually(beTruthy())

// after
expect(1 + 1).to.equal(2)
expect(1.2).to.beCloseTo(1.1, within: 0.1)
expect(3).to.beGreaterThan(2) // more verbose, but discoverable
expect("seahorse").to.contain("sea")
expect(["Atlantic", "Pacific"]).toNot.contain("Mississippi")
expect(ocean.isClean).toEventually.beTruthy()

Having smarter auto-completion can be invaluable when first using a project and can help users discover APIs that they may have otherwise never used.

stephencelis avatar Nov 16 '15 22:11 stephencelis

Definitely agree! This changes the custom matcher interface, which means a major version bump for Nimble. Still, I think the improvement in autocompletion is well worth it.

modocache avatar Nov 16 '15 22:11 modocache

Yeaaaaahhhhhhhhhhh

orta avatar Nov 16 '15 22:11 orta

It might not even have to be a breaking change immediately. We could add the protocol extension layer and call the module functions from there. The question is if Swift will resolve func to() vs. var to.

stephencelis avatar Nov 16 '15 22:11 stephencelis

orta avatar Nov 16 '15 22:11 orta

It might not even have to be a breaking change immediately. We could add the protocol extension layer and call the module functions from there.

Brilliant! :100:

modocache avatar Nov 16 '15 22:11 modocache

If no one else wants to noodle on this, I can take a stab this weekend :smile:

stephencelis avatar Nov 16 '15 22:11 stephencelis

Did a quick look at this and it doesn't look like var to and func to() can live in harmony :disappointed:

I see two paths forward…

  1. The backwards-compatible:

    // current style
    expect(array).to(beEmpty())
    expect(array).toNot(beEmpty())
    
    // new style
    expect(array).isEmpty()
    expect(array).isNotEmpty()
    
  2. The breaking:

    expect(array).to.beEmpty()
    expect(array).toNot.beEmpty()
    // or: expect(array).to.not.beEmpty()
    

Thoughts? The latter seems to fulfill the expectations (har har) of someone familiar with BDD, though the former is more succinct and active, kind of like changing a spec from it "should do this"

—to: it "does this".

stephencelis avatar Nov 17 '15 02:11 stephencelis

It's always possible to use something else other than to

jeffh avatar Nov 17 '15 02:11 jeffh

Sure. Ideas welcome if isEmpty()/isNotEmpty() and their ilk aren't desirable! Even is might work:

expect(array).is.empty()
expect(array).is.not.empty()

expect(foo).is.equalTo(bar)
expect(foo).is.not.equalTo(bar)

stephencelis avatar Nov 17 '15 03:11 stephencelis

What about no backwards-compatibility and a major dot release? Just an idea, feel free to push back on it.

modocache avatar Nov 17 '15 04:11 modocache

I'm cool with that. And on second thought is is a keyword, so it's not a friendly option. I'll try a breaking-change PR soon.

stephencelis avatar Nov 17 '15 04:11 stephencelis

Well, I mean, whatever @jeffh thinks is a good idea. I'm just throwing it out there. Seems like we have to compromise on the API to be backwards-compatible, so what does it look like if we remove that constraint?

modocache avatar Nov 17 '15 04:11 modocache

@modocache: I just don't see a big upside for the breaking change for existing users. I'm not against changing the entire API, but for what amounts to syntax change (with little end-user benefit), it doesn't seem as critical to break existing users. While I do understand the visual appeal of the dot-styled syntax and it's autocomplete benefits, I need more convincing to break every user of Nimble.

Maybe I just don't see it, but I'm not visualizing unique features, significantly better code (maintenance) as a benefit to this change. For example, an alternative to writing combinators:

// Adding `or` combinator to the existing system
expect(something).to(equal(2) || equal(4))

While I haven't really thought through it enough to make an opinionated stance to support combinators, the existing system makes it easy to extend towards that. But maybe enough people don't care about that kind of feature to be worth effort.

If there's something significantly more advantageous to doing dot-styled syntax, I could be convinced for a drop-the-world syntax replacement. Making (thousands of) developers to spend several hours updating their expectations is wasted engineering I prefer not to do at a drop of a hammer. I'd rather to a gradual migration instead (in parallel for several releases before removing the old).

Also, it needs more thorough evaluation based on what the swift compiler can handle (these existing generics have been painful to extend because of it).


Now my 2 cents about the syntactic style.

The first style:

expect(foo).isEmpty()
expect(foo).isNotEmpty()

Looks pretty expensive to implement and maintain. We'd need four methods for every matcher-expectation combination (to, toNot, toEventually, toEventuallyNot). Correct me if I'm wrong, but I'm not too thrilled about that.

The second style is much more to my liking overall. There are possible other styles, just throwing some ideas:

  • expect(x).should.equal(y) (old-rspec; gomega)
  • it(x).should.equal(y) - We'd have to make sure overloading works between Quick & Nimble
  • then(x).will.equal(y), then(x).should.equal(y), then(x).is.equalTo(y) - more cucumber-like

jeffh avatar Nov 17 '15 05:11 jeffh

@jeffh I'm happy to work on a backwards-compatible version, which is why I suggested it as a good first step :)

I may be in the minority, but I do think these kinds of changes go beyond syntax and visual appeal, and that the autocompletion benefits are invaluable to new users—especially new developers. I think Swift 2 introduced protocol extensions as an important step in its evolution as a language.

I also agree with you that using some kind of negating not (should vs. should.not) would be easier both maintainability- and discoverability-wise. It's just about finding the sweet spot, semantically.

I just read the combinator thread and might need more examples to understand the need for this kind of binary expectation. Instead of:

expect(someThing).to(startWith("https://")).or(startWith("http://"))

Why not the following?

expect(someThing).should.match("https?://")

stephencelis avatar Nov 17 '15 05:11 stephencelis

Turns out it's a moot point. My earlier spike quickly led to an unhelpful compile-time error that was hiding the root of the problem. Both can live in harmony after all! :smile:

stephencelis avatar Nov 17 '15 11:11 stephencelis

Sorry for the slow response. Still recovering from an illness :frowning:.

#176: Was requesting matching IPv6 or IPv4 addresses (and not just by http(s)). While you can use a conditional regex, it gets far less pretty:

expect((device.primaryIPAddress =~ RegularExpressions.IPv4Pattern) || (device.primaryIPAddress =~ RegularExpressions.IPv6Pattern)).to.beTruthy()

A matcher framework like Hamcrest uses combinators to build a feature-rich matcher library. It's more flexible at the cost of less discoverability (because it's all function composition):

// java
assertThat(responseString, either(containsString("color")).or(containsString("colour")));

In my opinion, good architecture of software is keeping the most options available regardless about what design patterns are used. I understand people love protocol extensions as a new paradigm, but it seems mostly the same to me -- granted, with better discoverability semantics. Just saying it's an evolution to a language is interesting to note, but it's just an appeal to authority. Language designers can make mistakes too.


Here's what I've understand around the tradeoffs of the dot-syntax:

Pros:

  • It's less syntactically noisy
  • It's more discoverable for new users (eg - autocomplete)

Cons:

  • Disallows the creation of matcher combinators

@stephencelis, that's for discovering that both can live side by side. It's good to hear :+1:.

jeffh avatar Nov 28 '15 20:11 jeffh

I don't see any reason why protocol extension-based syntax would disallow matcher combinators.

As an example:

expect(device.primaryIPAddress).to.match(IPv4Pattern).or.match(IPv6Pattern)

stephencelis avatar Nov 29 '15 21:11 stephencelis

If we look at that, it's ambiguous meaning:

// a.
expect(device.primaryIPAddress).to.match(IPv4Pattern)
// b.
expect(device.primaryIPAddress).to.match(IPv4Pattern).or.match(IPv6Pattern)

How does Nimble know which form (a or b) is executing? Is it ok to perform an assertion during the invocation to match? That would invalidate the purpose of the or. Alternatively, you'll need some wrapping call around the grammar (or only use operators):

// || operator takes both args as autoclosures to capture both matchers:
expect(device.primaryIPAddress).to.match(IPv4Pattern) || 
   expect(device.primaryIPAddress).to.match(IPv6Pattern)

A combinator needs to control when it's child matchers execute. I don't think that syntax can cover it.

jeffh avatar Dec 03 '15 08:12 jeffh

Swift type-checking could take care of it and correctly route things. The assertion would build lazily and be performed at the end of the statement.

stephencelis avatar Dec 03 '15 14:12 stephencelis

That requires double the implementations for matcher writers. But I think you've convinced me enough. I'll merge that PR once it's a good state :+1:

jeffh avatar Dec 09 '15 06:12 jeffh

:smile: Yay! Have you had a chance to read over #218 and address the concerns I brought up?

stephencelis avatar Dec 09 '15 12:12 stephencelis

Hey @stephencelis,

Sorry for worst turn-around on record. I'll look to try and get the PR up-to-date in the coming weeks.

I'm still interested in getting this into Nimble. I was wondering what were you're thoughts on getting satisfyAnyOf to function as a protocol extension?

jeffh avatar Oct 18 '16 05:10 jeffh

No problem! Open source should ideally be fun and low-stress (which I know open threads can rarely be). If you get this across the finish line, great!

I wasn't familiar with satisfyAnyOf, but imagine the || version would still work:

expect(82).to.beLessThan(50) || beGreaterThan(80)

And, more verbosely (without the free function):

expect(n).to.beLessThan(50)
  || expect(n).to.beGreaterThan(80)

stephencelis avatar Oct 24 '16 13:10 stephencelis

At this point, I don't think this is worth doing. It might have been more useful years earlier in the project. Introducing this much of a syntax change to Nimble, nearly 9 years since its original release, would not be worthwhile.

younata avatar Apr 04 '23 06:04 younata