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

Compile Time Mismatch Errors & centerAnchors #18

Conversation

nevillco
Copy link
Contributor

@nevillco nevillco commented Sep 20, 2016

Fixes #8 and #13:

  • Creates AnchorPair struct that creates constraints along 2 edges (or both centers)
  • Removes need for LayoutEdge enum (downside to this: "transforming" constraints is handled case-by-case)
  • EdgeAnchors composes from horizontalAnchors and verticalAnchors
  • ConstraintBuilder and EdgeConstraints (renamed to ConstraintGroup) add support for center anchors

nevillco added 3 commits September 20, 2016 14:49
…ors when constraining incompatible edge groups
…ssertion in ConstraintBuilder can stay

-  naming convention
… and support centerX/centerY pair

- Generalized EdgeConstraints -> (now ConstraintGroup) struct as above
- Removed need for  enum (downside to this: coding the inversion of constants on a case-by-case basis)
@nevillco nevillco force-pushed the bugfix/nevillco/Type-Mismatches branch from b2c69db to c9883d1 Compare September 21, 2016 14:09
@nevillco nevillco changed the title Bugfix/nevillco/Type Mismatches Compile Time Mismatch Errors & centerAnchors Sep 21, 2016
@KingOfBrian
Copy link
Contributor

Looks good @nevillco ! I feel like there's something we can do to remove the ConstraintBuilder or make it more specialized. Any thoughts @jvisenti ?

var constraintBuilder = ConstraintBuilder(horizontal: >=, vertical: >=)
public struct EdgeAnchors: LayoutAnchorType {
let horizontalAnchors: AnchorPair<NSLayoutXAxisAnchor, NSLayoutXAxisAnchor>
let verticalAnchors: AnchorPair<NSLayoutYAxisAnchor, NSLayoutYAxisAnchor>
Copy link
Contributor

Choose a reason for hiding this comment

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

#req change to var

}

}

// MARK: - Constraint Builders

private struct ConstraintBuilder {
struct ConstraintBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

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

#? Can we fileprivate this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Accidentally made this a review. It's non-blocking of course, but let's address the other questions on this branch before merge.

Copy link
Contributor Author

@nevillco nevillco Sep 22, 2016

Choose a reason for hiding this comment

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

We could, but my understanding of Swift 3 access control is we'd need to make it fileprivate within #if swift (>=3.0) and duplicate the method as private within the else, and do the same thing for AxisAnchors’ and EdgeAnchors’ constraints functions that take a ConstraintBuilder as a parameter. Is this correct, and if so, is it worth the extra code?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I'm not sure if it really matters since these aren't marked as public and therefore can't be accessed outside the module anyway. If someone drags in the .swift, they'll have access outside the file, but most people will probably incorporate via Cocoapods

@jvisenti
Copy link
Contributor

jvisenti commented Sep 21, 2016

My comment got deleted so here's what it said roughly:

@nevillco These changes look good to me, my only concern is that now ConstraintGroup is far too bloated, and I'm not sure how to pare it down due to the generality of AnchorPair. For example, someone could do

let constraint = (v1.horizontalAnchors == v2.horizontalAnchors).centerX

which doesn't really make sense. This produces a nil constraint, and not a crash, but I'd like to prevent it altogether. Thoughts on this @KingOfBrian ?

@KingOfBrian
Copy link
Contributor

Yea, I agree, but I don't think it's a new issue, it's just worse with the addition of Center.

We could create EdgeGroup, AxisGroup and only return the constraints that are populated, but the property names on AxisGroup might get tricky, as it would be first and second. It would be awesome though because we could remove the nullability on the result.

@jvisenti
Copy link
Contributor

Yeah it's not a new issue, just exacerbated by centerAnchors, and will only get worse.

@nevillco
Copy link
Contributor Author

nevillco commented Sep 21, 2016

Yeah, I agree on the issue, but as a user I'm not sure I like the tradeoff of the ambiguous property names in AxisGroup in lieu of the nil check. I'll give that solution a look.

@nevillco nevillco force-pushed the bugfix/nevillco/Type-Mismatches branch 3 times, most recently from 6c4b7ab to b842159 Compare September 21, 2016 18:30
- EdgeAnchors let -> var
- Split ConstraintGroup into EdgeGroup and AxisGroup, eliminating nullability but gives ambiguous constraint names in AxisGroup
- Indentation fixes
@nevillco nevillco force-pushed the bugfix/nevillco/Type-Mismatches branch from b842159 to 369d543 Compare September 22, 2016 12:30
@KingOfBrian KingOfBrian merged commit 4a01f60 into bugfix/Swift3.0-Swift2.3-Compatability Sep 22, 2016
@KingOfBrian KingOfBrian deleted the bugfix/nevillco/Type-Mismatches branch September 22, 2016 18:52
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

3 participants