Created summary cell #266
Created summary cell #266
Conversation
|
👀 |
There was a problem hiding this comment.
I've started reading the PR and then stumbled on something really bad to the point where it is not only unexpected but also greatly dangerous. The build fails but that is hidden by xcpretty.
I have already highlighted places where some improvement is needed (don't worry about conflicts as we're going to handle them in an easy way and I doubt they'll be a problem in this case).
Please, either apply the required changes to repair xcpretty or drop it. I don't care abut pretty printing the build as much as I care about having the build fail when it should.
(This is actually a great opportunity to fix this issue, since we know that the build is failing and we are clearly in a case in which xcpretty is making it succeed nonetheless.)
So, to summarise:
-
please either remove
xcprettyor make it work (I don't care aboutxcprettyFWIW) -
please apply the changes I've mentioned above, and hopefully they will be enough to repair the build
-
I'd like to take a second look at how the code changed
Thanks!
| [super setSelected:selected animated:animated]; | ||
|
|
||
| // Configure the view for the selected state | ||
| } |
There was a problem hiding this comment.
The two methods above seem to call a superclass; how about we just omit them?
| @@ -1,12 +1,14 @@ | |||
| #import "TestSummaryTableViewController.h" | |||
| #import "TestSummaryViewController.h" | |||
There was a problem hiding this comment.
Are you sure this name change is correct?
| #import "ReachabilityManager.h" | ||
| #import "Tests.h" | ||
| #import "UploadFooterViewController.h" |
There was a problem hiding this comment.
I believe this file is added in another PR, no?
| @@ -16,16 +18,17 @@ - (void)viewDidLoad { | |||
| [self.navigationController.navigationBar setBackgroundImage:[UIImage new] forBarMetrics:UIBarMetricsDefault]; | |||
| self.tableView.tableFooterView = [UIView new]; | |||
| [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(resultUpdated:) name:@"resultUpdated" object:nil]; | |||
| [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(reloadMeasurements) name:@"uploadFinished" object:nil]; | |||
There was a problem hiding this comment.
I guess emitting this notification is fine, even though we've not implemented the upload of measurements yet.
There was a problem hiding this comment.
Yet, I'd remove this code and reintroduce it later.
| else { | ||
| self.tableFooterConstraint.constant = 0; | ||
| [self.tableView setNeedsUpdateConstraints]; | ||
| } |
There was a problem hiding this comment.
Also this block of code does not belong here. Please remove.
| UploadFooterViewController *vc = (UploadFooterViewController * )segue.destinationViewController; | ||
| [vc setResult:result]; | ||
| [vc setUpload_all:true]; | ||
| } |
There was a problem hiding this comment.
Also this block of code does not belong here; please remove.
|
This can't be merged as it will break master. Storyboard conflict. Also is strictly connect with table rows where I added the upload icon. |
|
Please, reopen and fix the build. We can cherry pick the commit somewhere else. My job is hopeless if I cannot trust the build system at Travis to do its job. |
|
I want to do some more on my own on this PR, even if we eventually don't merge it. I think at this point it's important that I feel the pain of the storyboard myself. |
|
Pain was felt. We can now close this PR. |
No description provided.