Added UIView, NSView and CGRect extensions #38

Sweeper777 · 2017-01-12T07:03:43Z

I added some things that make dealing with positions and sizes easier. e.g. you can easily change a view's position by setting its x and y properties.

tbaranes

Looks good! I let you a few comments about the README, otherwise that's a cool add 👍

tbaranes · 2017-01-12T07:07:17Z

README.md

+let rect = CGRect(x: 10, y: 20, width: 30, height: 40)
+let widerRect = rect.with(width: 100)
+let tallerRect = rect.with(height: 100)
+let rectAtAnotherPosition = rect.with(x: 100).with(y: 200)


Can you add a comment // result printed once changed for each of them?

What do you mean by "result printed once changed"? I didn't have any print statements.

For example:
let widerRect = rect.with(width: 100) // x: 10, y: 20, width: 130, height: 140

tbaranes · 2017-01-12T07:10:28Z

README.md

+aView.width -= 10 // make the view narrower
+aView.height -= 10 // make the view shorter 
+```
+
 It will iterate on all the subviews of the view, and use the text / placeholder as key in `NSLocalizedString`.


That comment must be below Automates your localizables and above the new section.

tbaranes · 2017-01-12T07:11:20Z

README.md

@@ -1090,6 +1108,14 @@ mySwitch.toggle()
 aView.convertLocalizables()
 ```

+**Change the frame of the view easily**
+```swift
+aView.x += 100 // move  to right


Can you add a var aView: UIView to make the example more concrete?

tbaranes · 2017-01-12T07:11:31Z

README.md

@@ -1221,6 +1247,14 @@ view.addGestureRecognizer(blockTapGesture)
 aView.convertLocalizables()
 ```

+**Change the frame of the view easily**
+```swift
+aView.x += 100 // move  to right


Can you add a var aView: NSView to make the example more concrete?

tbaranes · 2017-01-12T07:11:52Z

README.md

+aView.width -= 10 // make the view narrower
+aView.height -= 10 // make the view shorter 
+```
+
 It will iterate on all the subviews of the view, and use the text / placeholder as key in `NSLocalizedString`.


Same here: that comment is for the above section / example

tbaranes · 2017-01-12T07:14:06Z

CHANGELOG.md

+public func with(x: CGFloat) -> CGRect
+public func with(y: CGFloat) -> CGRect
+public func with(width: CGFloat) -> CGRect
+public func with(height: CGFloat) -> CGRect


What do you think to add func with(origin: CGPoint) -> CGRect and func with(size: CGSize) -> CGRect, can it be useful? 🤔

Yeah, that's a good idea! I didn't think of that.

tbaranes · 2017-01-12T07:42:40Z

Also, can you add some tests? It's straight forward, should never fail, but we are never careful enough!

Sweeper777 · 2017-01-12T07:44:02Z

Ok. I forgot about them!

Sweeper777 · 2017-01-12T08:13:34Z

I have added everything (hopefully)! Can this merge?

tbaranes

I have a last request changes: can you duplicate the UIViewExtensionsTests for macOS as well?

Sweeper777 · 2017-01-12T08:27:06Z

Done!

tbaranes

Perfect! Feel free to merge it once travis is green 🎉
Thanks for that PR!

tbaranes · 2017-01-12T08:34:40Z

There's a few SwiftLint violations which is making travis failing.
Since it's only due to x, and y being too short, you can exclude that case by adding the following lines in to .swiftlint.yml:

variable_name:
  min_length: 2
  excluded: 
   - x
   - y

By the way, I suggest you ton install it, it's a nice linter and you will have the warnings / errors directly in xcode when building 😬

Sweeper777 · 2017-01-12T08:47:09Z

Is .swiftlint.yml a file I have to create? I can't find it in the repo.

tbaranes · 2017-01-12T08:53:52Z

The configuration file for the core is there: ./SwiftyUtils/.swiftlint.yml
For the example, it's there (no need to change this one, just for your information): ./SwiftyUtils Example/.swiftlint.yml

tbaranes · 2017-01-12T12:23:54Z

I went ahead and fix it (+ bump a few others warnings due to the latest release) in #39
That was a work I have to do since a few time, so...

You will just have to rebase your PR once I merged the other, then, everything should be ok.

Sweeper777 · 2017-01-12T13:10:49Z

Do you mean that once #39 is merged, I can press the bottom button there? Is that what rebase means?

tbaranes · 2017-01-12T13:50:30Z

We have to ensure first that travis is passing as expected (should be no issue there), but in order to test that, you have to rebase without merging, which means pull the lasts commits from master in your branch. Using Rebase and merge from github will directly merge the PR in master without waiting for travis.

If you are using a terminal:

git fetch origin            # Updates origin/master
git rebase origin/master    # Rebases current branch onto origin/master
git -f push                 # Force push to override git history (your commits must be the recent ones)

If you are using an application (tower, source tree...), they have generally an option build in to do it.

Let me know if you need more information!

Sweeper777 · 2017-01-12T13:56:14Z

Ok, seems easy. I'll do that once I get back!

Sweeper777 · 2017-01-12T15:58:29Z

It is telling me that git -f push has an unknown option -f. Should it be git push -f instead?

tbaranes · 2017-01-12T16:05:13Z

Yeah, should be that, my bad

tbaranes · 2017-01-12T16:07:22Z

All good 👍 Feel free to merge that PR once travis is done.
Good job here, thanks again!

tbaranes · 2017-01-12T17:06:47Z

Seems to be an issue left:

❌ /Users/travis/build/tbaranes/SwiftyUtils/SwiftyUtils/SwiftyUtils macOS Tests/NSViewExtensionsTest.swift:14:20: use of unresolved identifier 'NSView'

NSViewExtensionsTest.swift should target only macOS, seems to be currently iOS too

Just in case you don't know: you can click on "Details" (like in the below screenshot) to see the travis status (current build, success, failure... and details about what did fail)

Sweeper777 · 2017-01-12T17:20:34Z

I think I fixed it. I built the target and ran the test. They all succeeded so it should be alright. Right?

tbaranes · 2017-01-12T17:22:17Z

Travis will tell us soon enough 😬
But yeah, if it passes in local, it should be ok for travis

tbaranes · 2017-01-12T17:43:07Z

@Sweeper777 Side note for the next time (I pushed directly in your branch this time):

Classes added in the Core must target tvOS and watchOS as well (same for the CoreTests)
When adding a new section in the README, you have to update the summary as well. Also, the README is sorted alphabetically

Otherwise 💯

Sweeper777 · 2017-01-12T18:15:30Z

Is it normal for Travis to take so long? Is something wrong?

tbaranes · 2017-01-12T18:32:57Z

It can, that's depending how many build Travis is handling at the same time, and their structures status. Sometimes it's quite long...

Sweeper777 · 2017-01-13T06:13:29Z

Oops! I was on my phone and I accidentally merged it! I'm so sorry! Travis seems to still fail saying that tests failed.

tbaranes · 2017-01-13T07:02:50Z

The build failed of travis wasn't due to test but a timeout, it happened, the only solution in this case is to start again the build which has failed. I think you have the rights to do that since you are a contributor.

For the next time if there's really a test which has failed and a PR is accidentally merged, the good move is just to rollback to the last master commit before the merge, you can find the command line on SO.

For now, it's all good 👍

Sweeper777 · 2017-01-14T11:47:35Z

Maybe update to 0.7.0 now?

tbaranes · 2017-01-14T12:45:03Z

👌Will do it this weekend or next week

tbaranes requested changes Jan 12, 2017

View reviewed changes

tbaranes reviewed Jan 12, 2017

View reviewed changes

tbaranes requested changes Jan 12, 2017

View reviewed changes

tbaranes approved these changes Jan 12, 2017

View reviewed changes

Sweeper777 added 7 commits January 12, 2017 15:58

added CGRectExtensions

42160c6

Added UI and NSView extensions

e453f4e

updated changelog

8e691b6

updated README

af13e89

added proposed changes

0b5064c

added tests

492c7d4

added macOS Tests

b77ad28

Sweeper777 force-pushed the uiview&cgrect branch from 0a617f5 to b77ad28 Compare January 12, 2017 16:05

changed the test file's target membership

272ab08

tbaranes added 2 commits January 12, 2017 18:40

Add CGRectExtension to tvOS and watchOS

86b3dfb

Update README summary

b667cc8

Sweeper777 merged commit f230c0b into master Jan 13, 2017

tbaranes deleted the uiview&cgrect branch January 13, 2017 07:01

Added UIView, NSView and CGRect extensions #38

Added UIView, NSView and CGRect extensions #38

Conversation

Sweeper777 commented Jan 12, 2017

tbaranes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbaranes Jan 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbaranes commented Jan 12, 2017

Sweeper777 commented Jan 12, 2017

Sweeper777 commented Jan 12, 2017

tbaranes left a comment

Choose a reason for hiding this comment

Sweeper777 commented Jan 12, 2017

tbaranes left a comment • edited

Choose a reason for hiding this comment

tbaranes commented Jan 12, 2017 • edited

Sweeper777 commented Jan 12, 2017

tbaranes commented Jan 12, 2017

tbaranes commented Jan 12, 2017

Sweeper777 commented Jan 12, 2017

tbaranes commented Jan 12, 2017 • edited

Sweeper777 commented Jan 12, 2017

Sweeper777 commented Jan 12, 2017

tbaranes commented Jan 12, 2017

tbaranes commented Jan 12, 2017 • edited

tbaranes commented Jan 12, 2017 • edited

Sweeper777 commented Jan 12, 2017

tbaranes commented Jan 12, 2017

tbaranes commented Jan 12, 2017

Sweeper777 commented Jan 12, 2017

tbaranes commented Jan 12, 2017

Sweeper777 commented Jan 13, 2017

tbaranes commented Jan 13, 2017

Sweeper777 commented Jan 14, 2017

tbaranes commented Jan 14, 2017

tbaranes Jan 12, 2017 •

edited

tbaranes left a comment •

edited

tbaranes commented Jan 12, 2017 •

edited

tbaranes commented Jan 12, 2017 •

edited

tbaranes commented Jan 12, 2017 •

edited

tbaranes commented Jan 12, 2017 •

edited