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

Autoboxing for scalar/struct attribute values #62

Merged
merged 9 commits into from
Apr 30, 2014

Conversation

nickynick
Copy link
Contributor

Implemented my idea from #61. Allows you to write code like this (without using number literals and other ugly stuff):

make.top.equalTo(42);
make.height.equalTo(20);
make.size.equalTo(CGSizeMake(50, 100));
make.edges.equalTo(UIEdgeInsetsMake(10, 0, 10, 0));

Old code still works fine :)

Things I'm gonna do before this is ready for merging:

  • Add tests
  • Update/add docs
  • Update examples
  • Implement the same thing for offset? This would effectively ditch centerOffset and pointOffset.

@cloudkite
Copy link
Contributor

@nickynick looks cool 👍

My only reservation is that this means that equalTo, greaterThanOrEqualTo, lessThanOrEqualTo are now globals. Which means any existing or new methods on other classes with the same name equalTo etc will not be accessible as the #define equalTo will over shadow their method.

@cloudkite
Copy link
Contributor

However to be on the safe side maybe we can default to using mas_equalTo, mas_greaterThanOrEqualTo, mas_lessThanOrEqualTo

Unless the developer opts in to the unprefixed version by adding a define to their prefix.pch
#define MAS_SHORTHAND_GLOBALS

We would need to leave in the existing methods equalTo, greaterThanOrEqualTo, lessThanOrEqualTo so that they can continue to use these methods even if they do not opt-in to the auto-boxing unprefixed versions of these methods

What are your thoughts?

@nickynick
Copy link
Contributor Author

@cloudkite Yup, that's what I'm thinking about right now too.

One possibility would be to do something like this (similar to MAS_SHORTHAND):

#ifndef MAS_DISABLE_AUTOBOXING
#define equalTo(...)                 _equalToWithRelation(MASBoxValue((__VA_ARGS__)), NSLayoutRelationEqual)
#define greaterThanOrEqualTo(...)    _equalToWithRelation(MASBoxValue((__VA_ARGS__)), NSLayoutRelationGreaterThanOrEqual)
#define lessThanOrEqualTo(...)       _equalToWithRelation(MASBoxValue((__VA_ARGS__)), NSLayoutRelationLessThanOrEqual)
#endif

This would also require to provide actual implementation for these methods for non-define fallback.

Perhaps there is another solution, I'm looking for it.

@nickynick
Copy link
Contributor Author

I went with MAS_SHORTHAND_GLOBALS approach. Looks completely safe and sweet now!

Added macro for offset too, being able to supply it with any type of offset feels convenient.

I also decided to take the liberty and did a little cleanup by means of moving implementations of semantically identical abstract methods to MASConstraint.

@nickynick
Copy link
Contributor Author

@cloudkite I'm pretty much done :) Please check if there's anything else to fix for me when you have some spare time.

@cloudkite
Copy link
Contributor

@nickynick nice work! Sorry I'm a bit swamped at work at the moment, but will try my best to review it as soon as possible :)

@nickynick
Copy link
Contributor Author

@cloudkite No probs :)

@@ -171,6 +171,35 @@

@end


@interface MASConstraint (Private)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe move this into its own file? And then import it in the necessary .m files then it will be truly private as it wont be exposed by masonry.h

Also once its private we wont have to prefix with _

@cloudkite
Copy link
Contributor

Works very nicely 👍 Just one small comment. Once it's merged I will try finish off #56 asap so we can push out a new cocoapods version

@nickynick
Copy link
Contributor Author

They are not really private because this is what macros actually expand to, therefore they should be visible in public scope. It's just that you are really not supposed to be calling them manually. Maybe it would make sense to change category name from Private to AutoboxingSupport or something like that?

@cloudkite
Copy link
Contributor

yeah sounds good. Should we drop the underscore prefix? Given apple states they reserve it for their own use?

@nickynick
Copy link
Contributor Author

I'd leave underscores because otherwise these methods will pop out in autocompletion which I would not like because they are really not supposed to be used directly :) Regarding Apple, well, we are subclassing NSObject so I honestly don't think we are violating anything.

So, I'd make a private header and move there updateExisting, delegate and setLayoutConstantWithValue: which clients don't have any business with, and would leave _valueOffset and _equalToWithRelation together with other macro stuff. What do you think?

@cloudkite
Copy link
Contributor

I don't think NSObject subclasses are exempt from this convention. NSObject is likely to have a whole bunch of private methods that are prefixed. Its most likely a non-issue given that it is unlikely that NSObject would have methods with the same name.

However just thinking its better to be on the safe side, won't they appear in autocomplete as _equalToWithRelation etc anyway just with since they are public?

@cloudkite
Copy link
Contributor

I guess just trying to guard against possibility that apple becomes stricter with this convention in future and scans binaries for any underscore prefixed methods

@nickynick
Copy link
Contributor Author

won't they appear in autocomplete as _equalToWithRelation etc anyway just with since they are public?

Sure they will, but only if you start typing with underscore :)

Anyway, let's just unprefix valueOffset() (it's not that bad in public, I guess) and hide equalToWithRelation(). Let's keep it simple and safe :)

@cloudkite
Copy link
Contributor

looks great! Thanks again for the contribution 👍

cloudkite added a commit that referenced this pull request Apr 30, 2014
Autoboxing for scalar/struct attribute values
@cloudkite cloudkite merged commit 5fcd1bd into SnapKit:master Apr 30, 2014
@nickynick nickynick deleted the equalTo-autoboxing branch May 1, 2014 20:36
dan-rodrigues pushed a commit to dan-rodrigues/Masonry that referenced this pull request Oct 18, 2014
Autoboxing for scalar/struct attribute values
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.

3 participants