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

Various fixes #79

Merged
merged 12 commits into from Sep 6, 2016
Merged

Various fixes #79

merged 12 commits into from Sep 6, 2016

Conversation

gfontenot
Copy link
Collaborator

@gfontenot gfontenot commented Aug 25, 2016

This includes a few things. Some minor (formatting, SwiftCheck update), some
larger (Extension API across all targets, updates for newest version of Swift
3)

Don't know how I feel about these precedence groups, tbh. I don't think I feel
super great about them. Our tests are also completely broken, since it's
apparently impossible to resolve the differences between how we're choosing to
define these operators and how SwiftCheck defines them. No idea what to do
about this.

Fixes #59
Fixes #78

gfontenot added 5 commits Aug 3, 2016
We were missing this for the tvOS target.
@NoEscape is now the default, so we need to be using @escaping to indicate
function params that might escape. This change was led by the compiler, which
is nice.
These remove the need for the explanitory comments about what the specific
precedence means. Unfortunately, there's no `equalTo` operator for these, so
we can't actually define these as accurately as I'd like. There's also no
ability to define multiple `higherThan` or `lowerThan` precedence groups,
which means we can't ensure that our `AlternativePrecedence` is positioned
correctly above our monadic precedences.

All in all, I'm not sure how I feel about this. I'm less confident in the
positioning of these groups now and I'm unclear on how changes will affect
consuming code.
@@ -713,6 +726,7 @@
51DE8A2B1BAB36E600124320 /* Release */ = {
isa = XCBuildConfiguration;
buildSettings = {
APPLICATION_EXTENSION_API_ONLY = NO;
Copy link
Contributor

@hyperspacemark hyperspacemark Aug 25, 2016

Choose a reason for hiding this comment

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

Why revert this? We should be able to use Runes in extensions without warnings as Runes doesn't use any extension unsafe API.

Copy link
Collaborator Author

@gfontenot gfontenot Aug 25, 2016

Choose a reason for hiding this comment

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

This is for the test bundle. I'm actually setting this to YES at the project level so that all of the frameworks have it set properly, but the tests need to have this set to NO. It's a confusing diff.

@gfontenot
Copy link
Collaborator Author

@gfontenot gfontenot commented Aug 25, 2016

Seriously tempted to just kill the tests here and have CI simply build the framework for the various targets.

// Lower precedence than `<^>`, `<*>`, `*>`, `<*`
precedence 120
}
infix operator <|>: AlternativePrecedence
Copy link
Contributor

@hyperspacemark hyperspacemark Aug 25, 2016

Choose a reason for hiding this comment

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

AlternativePrecedence reads as extremely vague to me. Alternative to what? Any reason to not go with, like, LeftAssociativeBinaryPrecedence?

Copy link
Collaborator Author

@gfontenot gfontenot Aug 25, 2016

Choose a reason for hiding this comment

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

Yeah, fair point. This operator is from the Alternative typeclass in Haskell, which is why I named it as such. I'm totally open to dropping that baggage though.

Using ComparisonPrecedence here didn't actually work the way we wanted.
Apparently, ComparisonPrecedence doesn't actually have an associativity
defined, so chaining these together doesn't actually work. To fix this we can
define our own precedence groups (inspired by those defined in SwiftCheck) and
set up a precedence tree.

Unfortunately since our tests are broken, it only became apparent when we
integrated this lib into Argo.

precedencegroup AlternativePrecedence {
associativity: left
higherThan: ApplicativeSequencePrecedence
Copy link
Collaborator Author

@gfontenot gfontenot Aug 25, 2016

Choose a reason for hiding this comment

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

Oops I lied, this should be lower.

We need to explicitly put the monadic precedences at the bottom of the stack.
I'm unclear on how this will affect things.

Also fixing the precedence for Alternative to match the precedence set in
Haskell.
@jshier
Copy link

@jshier jshier commented Aug 25, 2016

@gfontenot I meant to post it earlier, but I ported Runes to beta 6 for my own projects, but didn't open a PR since I couldn't run the tests. Here's my Runes.swift file. As you can see I built two custom precedence groups and used system groups for the others. I'm able to use this version just fine in my projects, replacing the earlier operator definitions.

@gfontenot
Copy link
Collaborator Author

@gfontenot gfontenot commented Aug 25, 2016

@jshier Ah interesting. I started running into problems when trying to use this with Argo since I needed to define the precedence of <| based on the applicative operators. Did you run into that?

I think this is hooked up the way we'd expect it to be hooked up now. Argo seems happy, at least.

@jshier
Copy link

@jshier jshier commented Aug 25, 2016

@gfontenot I didn't, though I only ported the operators with the intention of matching the previous numeric levels.

gfontenot added 4 commits Aug 29, 2016
I'm not too worried about getting this exactly right since we're not exporting
it publicly. I just want to make sure it's got the correct precedence for the
tests.
They removed their custom operators! Our tests build again (and should be much
less fragile moving forward)!
Not sure if this will help/hurt, but I can't help but feel like defining these
in terms of the stdlib instead of in their own tree will help with ambiguous
precedences.
@gfontenot
Copy link
Collaborator Author

@gfontenot gfontenot commented Aug 29, 2016

I think this is probably ready to roll, but I'd love a sanity check from other interested parties. ping @thoughtbot/ios

Turns out these need to be unique! Fun stuff. To reduce the risk of conflict
here lets go ahead and prefix our precedence group names with our package
name.

This sucks.
@tonyd256
Copy link

@tonyd256 tonyd256 commented Sep 6, 2016

👍

@gfontenot gfontenot merged commit 5c07ea7 into master Sep 6, 2016
1 check passed
@gfontenot gfontenot deleted the gf-fixes branch Sep 6, 2016
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.

4 participants