Fixed some compiler warnings #2426

randallli · 2017-11-14T22:14:03Z

I copied the warnings/errors for our components for our examples and fixed some errors.

This reverts commit 91f0480.

…creating a new one.

ianegordon · 2017-11-14T22:28:33Z

components/Collections/examples/CollectionsEditingExample.m

-  BOOL isEditing = self.editor.isEditing;
-  [self updatedRightBarButtonItem:!isEditing];
-  [self.editor setEditing:!isEditing animated:YES];
+  if ([sender isEqual:self.navigationItem.rightBarButtonItem]) {


This seems strange to me. We are querying the sender, but it makes not difference in the body of the if clause. If another control were to use this action if would fail, which would be surprising.
If we don't care what the sender it, drop that parameter.
Here and below.

Agreed that I'm not certain how important it is for us to be checking the sender in these methods. If the intent was simply to make use of the sender then I would encourage us to simply mark the sender as __unused instead and keep the simpler logic we had before.

ok im going to drop the sender.
For other cases that require the parameters I will be putting an NSLog statement to silence the warning.

Nooooo. Mark the parameter as __unused.

Why is NSLog preferred over __unused? Can't you at least do something less verbose, like:

(void)unusedVariable;

We are intentionally trying to keep the examples 'simple'
Beginning engineers will likely know what NSLog() is. They are unlikely to be as familiar with __unused.

ianegordon · 2017-11-14T22:29:38Z

components/Dialogs/examples/DialogsAlertViewController.m

 }

- (IBAction)didTapShowAlert:(id)sender {
+- (IBAction)didTapShowAlert{


super-nit: space after method name, here and below.

ianegordon · 2017-11-14T22:31:06Z

components/FeatureHighlight/examples/FeatureHighlightShownViewExample.m

-  vc.titleText = @"Shown views can be interactive";
-  vc.bodyText = @"The shown button has custom tap animations.";
-  [self presentViewController:vc animated:YES completion:nil];
+  if ([sender isEqual:self.button]) {


Again, gating an action method on the sender, but not using the sender in the body of the method seems strange, and would make it harder to re-use this method in another control.

ianegordon · 2017-11-14T22:32:59Z

components/FlexibleHeader/examples/FlexibleHeaderUINavigationBarExample.m

+
+  [self.navigationItem setLeftBarButtonItem:backItem animated:YES];
+  [self.navigationItem setRightBarButtonItem:doneItem animated:YES];
+  [navBar setItems:[NSArray arrayWithObject:self.navigationItem] animated:YES];


Consider boxing the array init.
setItems @[self.navigationItem] animated:YES

ianegordon · 2017-11-14T22:35:39Z

components/Tabs/examples/supplemental/TabBarIconExampleSupplemental.h

-@property MDCAppBar *_Nullable appBar;
-@property UIScrollView *_Nullable scrollView;
-@property UIView *_Nullable starPage;
+@property(nonatomic) MDCTabBar *_Nullable tabBar;


super-nit: If we are touching these lines, can we move _Nullable into the @Property annotation?
@Property(nonatomic, nullable) MDCTabBar *tabBar;

jverkoey · 2017-11-14T22:50:06Z

components/AnimationTiming/examples/AnimationTimingExample.m

-                                     view.transform = CGAffineTransformIdentity;
-                                   }
-                                   completion:nil];
+        if (finished) {


This is a change in behavior, and I'm not confident that it's a desirable one. Consider reverting and marking finished as unused instead?

jverkoey · 2017-11-14T22:52:38Z

components/Collections/examples/CollectionsEditingExample.m

-  BOOL isEditing = self.editor.isEditing;
-  [self updatedRightBarButtonItem:!isEditing];
-  [self.editor setEditing:!isEditing animated:YES];
+  if ([sender isEqual:self.navigationItem.rightBarButtonItem]) {


Agreed that I'm not certain how important it is for us to be checking the sender in these methods. If the intent was simply to make use of the sender then I would encourage us to simply mark the sender as __unused instead and keep the simpler logic we had before.

jverkoey · 2017-11-14T22:55:29Z

It appears that many of the changes in this PR were initiated because of the unused arguments warning, sometimes subtly changing the behavior in ways that I think aren't desirable. It may be worth reconsidering whether each of the modifications would be better replaced with an __unused annotation.

ajsecord · 2017-11-16T13:02:10Z

We're killing the Wunused-parameter warning (see internal discussion), so we can we drop this PR?

ajsecord · 2017-11-16T13:10:31Z

#2448 is the PR to remove unused parameter warnings.

randallli · 2017-11-16T17:19:41Z

I can drop the unused edits but there are others that we still want. Type casting and others.

addressed all concernes

randallli · 2017-11-17T21:09:31Z

Note: #2456 changes the warnings that we have.

no addressing __unused warnings in this PR

randallli · 2017-11-17T20:34:37Z

components/Dialogs/examples/DialogsAlertViewController.m

-                                                   handler:^(MDCAlertAction *action) {
+  MDCAlertAction *okAction = [MDCAlertAction actionWithTitle:@"OK"
+                                                     handler:^(MDCAlertAction *action) {
                                                     NSLog(@"%@", @"OK pressed");


more spaces

randallli · 2017-11-17T20:34:51Z

components/Dialogs/examples/DialogsAlertViewController.m

-                                                   handler:^(MDCAlertAction *action) {
+  MDCAlertAction *okAction = [MDCAlertAction actionWithTitle:@"OK"
+                                                     handler:^(MDCAlertAction *action) {
                                                     NSLog(@"%@", @"OK pressed");


more spaces

randallli · 2017-11-17T20:36:41Z

components/FlexibleHeader/examples/FlexibleHeaderUINavigationBarExample.m

+
+  [self.navigationItem setLeftBarButtonItem:backItem animated:YES];
+  [self.navigationItem setRightBarButtonItem:doneItem animated:YES];
+  [navBar setItems:[NSArray arrayWithObject:self.navigationItem] animated:YES];


randallli · 2017-11-17T20:38:07Z

components/Tabs/examples/supplemental/TabBarIconExampleSupplemental.h

-@property MDCAppBar *_Nullable appBar;
-@property UIScrollView *_Nullable scrollView;
-@property UIView *_Nullable starPage;
+@property(nonatomic) MDCTabBar *_Nullable tabBar;


randallli · 2017-11-17T21:07:28Z

components/Dialogs/examples/DialogsAlertViewController.m

 }

- (IBAction)didTapShowAlert:(id)sender {
+- (IBAction)didTapShowAlert{


codecov-io · 2017-11-18T03:07:12Z

Codecov Report

Merging #2426 into develop will decrease coverage by 1.24%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2426      +/-   ##
===========================================
- Coverage    45.02%   43.77%   -1.25%     
===========================================
  Files          237      252      +15     
  Lines        23206    24039     +833     
  Branches      1999     2046      +47     
===========================================
+ Hits         10448    10523      +75     
- Misses       12748    13506     +758     
  Partials        10       10

Impacted Files	Coverage Δ
components/Tabs/src/private/MDCItemBarCell.m	`34.74% <0%> (-1.22%)`	⬇️
components/NavigationBar/src/MDCNavigationBar.m	`73.73% <0%> (-0.33%)`	⬇️
...nents/ActivityIndicator/src/MDCActivityIndicator.m	`17.61% <0%> (-0.18%)`	⬇️
...te/Icons/icons/ic_info/src/MaterialIcons+ic_info.m	`71.42% <0%> (ø)`	⬆️
components/ButtonBar/src/MDCButtonBar.m	`50.86% <0%> (ø)`	⬆️
...xtFields/src/private/MDCTextInputCommonFundament.h	`33.33% <0%> (ø)`	⬆️
...xtFields/src/private/MDCTextInputCommonFundament.m	`90.62% <0%> (ø)`	⬆️
components/BottomAppBar/src/MDCBottomAppBarView.m	`0% <0%> (ø)`	⬆️
...te/ShapeLibrary/src/MDCSlantedRectShapeGenerator.m	`0% <0%> (ø)`	⬆️
...rivate/ShapeLibrary/src/MDCCurvedCornerTreatment.m	`0% <0%> (ø)`	⬆️
... and 73 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30c9fcb...e6dda25. Read the comment docs.

randallli · 2017-11-21T21:45:36Z

PTAL

randallli · 2017-11-22T15:41:08Z

I still need approval from someone.
ping @ianegordon @jverkoey who requested changes.

* Added warnings to examples. * fixed some warnings * using button sender parameter. * use button sender in example * use button sender in example * removed sender from methods that don’t use it. * removed sender from methods that don’t use it. * use button sender in example * use button sender in example * use button sender in example * use button sender in example * Fixed init of header configurator to use passed in parameter * removed unused API * use button sender in example * Revert "Added warnings to examples." This reverts commit 91f0480. * fixed example: Using the view controllers navigationItem rather than creating a new one. * removed finished checks from animation blocks in examples. * reverted id sender checks * revert id sender check for Flexible header UINavigationBar * revert clug * revert clug * addressing nits * removed sender check for FeatureHighlightTypicalUseView * more nits

randallli added 15 commits November 13, 2017 13:16

Added warnings to examples.

91f0480

fixed some warnings

1b8df1d

using button sender parameter.

49bcdb9

use button sender in example

4a80e7c

use button sender in example

8925260

removed sender from methods that don’t use it.

c642e04

removed sender from methods that don’t use it.

b934edc

use button sender in example

5769ee2

use button sender in example

ac1b54f

use button sender in example

420e8d4

use button sender in example

5411430

Fixed init of header configurator to use passed in parameter

1e9c0d2

removed unused API

ba49eb8

use button sender in example

be7c247

Revert "Added warnings to examples."

ff1700f

This reverts commit 91f0480.

googlebot added the cla: yes label Nov 14, 2017

fixed example: Using the view controllers navigationItem rather than …

6677a3b

…creating a new one.

ianegordon previously requested changes Nov 14, 2017

View reviewed changes

jverkoey previously requested changes Nov 14, 2017

View reviewed changes

randallli added 7 commits November 17, 2017 15:15

removed finished checks from animation blocks in examples.

a0da361

reverted id sender checks

a8fd4f8

revert id sender check for Flexible header UINavigationBar

08c1900

revert clug

b87590d

revert clug

94a8c38

addressing nits

ce5e314

removed sender check for FeatureHighlightTypicalUseView

57296d6

more nits

e6dda25

randallli commented Nov 17, 2017

View reviewed changes

randallli mentioned this pull request Nov 17, 2017

[CocoaPods] Added warnings to examples. #2480

Merged

randallli requested a review from a team November 21, 2017 21:44

randallli assigned jverkoey Nov 22, 2017

jverkoey approved these changes Nov 22, 2017

View reviewed changes

ianegordon merged commit 2f29f00 into material-components:develop Nov 22, 2017

Fixed some compiler warnings #2426

Fixed some compiler warnings #2426

Uh oh!

Conversation

randallli commented Nov 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jverkoey commented Nov 14, 2017

Uh oh!

ajsecord commented Nov 16, 2017

Uh oh!

ajsecord commented Nov 16, 2017

Uh oh!

randallli commented Nov 16, 2017

Uh oh!

randallli commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Nov 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

randallli commented Nov 21, 2017

Uh oh!

randallli commented Nov 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

randallli commented Nov 17, 2017 •

edited

Loading

codecov-io commented Nov 18, 2017 •

edited

Loading