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

Convert TRTemperature to Temperature #141

Merged
merged 1 commit into from
Jul 3, 2015
Merged

Convert TRTemperature to Temperature #141

merged 1 commit into from
Jul 3, 2015

Conversation

tomkowz
Copy link
Contributor

@tomkowz tomkowz commented Jul 2, 2015

Converted class to Swift.


self.highTemperature = [[Temperature alloc] initWithFahrenheitValue:[JSON[@"temperatureMax"] integerValue]];
self.lowTemperature = [[Temperature alloc] initWithFahrenheitValue:[JSON[@"temperatureMin"] integerValue]];
Copy link

Choose a reason for hiding this comment

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

remove extra space

@tomkowz
Copy link
Contributor Author

tomkowz commented Jul 2, 2015

I did improvements and pushed with -f option. I hope Circle CI will handle the updated commit. Also rebased.

@tomkowz
Copy link
Contributor Author

tomkowz commented Jul 2, 2015

Something failed but not related to tests. Works locally as should work.

}

// MARK: NSObjectProtocol
override var description: String {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's go ahead and say this class conforms to Printable while we're at it. When we're swift only, it might be nice to be able to interpolate this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if I get your message correctly (correct me if wrong) but we inherits from NSObject now and this is the correct solution for now. When we finally move to swift all the code and Temperature class will not need to be visible in Objective-C code then we can drop NSObject inheritance and use Printable instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe he's saying to also add Printable now so that we don't have to worry about adding it in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that makes sense. Added.

@tomkowz
Copy link
Contributor Author

tomkowz commented Jul 3, 2015

@jakecraige @tonyd256
So, Is that ready to merge for you? Nobody merged this overnight. (6 hours)

@@ -1,6 +1,6 @@
@import CoreLocation;
#import "TRWeatherUpdate.h"
#import "TRTemperature.h"
#import "UnitTests-Swift.h"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is. This is umbrella header which contains headers for Temperature class for Objective-C. Everywhere where Swift class is used in Objective-C we have to import this one in testing or "Tropos-Swift.h" in Tropos target. This will be dropped in the future when all will be in Swift ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome. Thanks for the info.

@tomkowz
Copy link
Contributor Author

tomkowz commented Jul 3, 2015

@jakecraige, improved the code.

@jakecraige jakecraige merged commit 877990c into thoughtbot:master Jul 3, 2015
@jakecraige
Copy link
Contributor

Thanks @tomkowz it all looks good to me now. Even CircleCI is passing now. I wonder if they were having trouble earlier if if something you changed fixed it. Either way it's green!

Merged 👍

@tomkowz
Copy link
Contributor Author

tomkowz commented Jul 3, 2015

Cool ;) Will prepare another things to review for you guys so you're not bored ;)

@tomkowz tomkowz deleted the feature/trtemperature branch July 3, 2015 05:38
@tomkowz tomkowz mentioned this pull request Jul 3, 2015
This pull request was closed.
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