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.

Sign up for GitHub

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

Jump to bottom

Enables pedantic warnings and 'treat warnings as errors'. #62

Closed
wants to merge 5 commits into from
Closed

Enables pedantic warnings and 'treat warnings as errors'. #62

wants to merge 5 commits into from

Conversation

StevenEWright
Copy link
Collaborator

@StevenEWright StevenEWright commented Nov 24, 2016 •
edited by iainmcgin
Loading

This change is Reviewable

@StevenEWright
Copy link
Collaborator Author

@WilliamDenniss I can't see what the bitrise error is. Can you tell me?

@WilliamDenniss
Copy link
Member

Reviewed 12 of 12 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Source/iOS/OIDAuthorizationUICoordinatorIOS.h, line 37 at r1 (raw file):

/*! @brief The designated initializer.
    @param presentingViewController The view controller from which to present the
        \SFSafariViewController.

This slash is a Doxygen macro that will like out to Apple's docs. Did you intend to remove it?

Comments from Reviewable

@WilliamDenniss
Copy link
Member

Errors from bitrise:

▸ Building AppAuth/AppAuth [Debug]
▸ Check Dependencies
▸ Compiling OIDErrorUtilities.m
▸ Compiling OIDTokenUtilities.m
▸ Compiling OIDGrantTypes.m
▸ Compiling OIDTokenRequest.m
▸ Compiling OIDAuthState+IOS.m
▸ Compiling OIDTokenResponse.m
▸ Compiling OIDScopeUtilities.m
▸ Compiling OIDAuthorizationResponse.m
▸ Compiling OIDServiceConfiguration.m
▸ Compiling OIDAuthState.m
▸ Compiling OIDAuthorizationService.m
▸ Compiling OIDAuthorizationUICoordinatorIOS.m
▸ Compiling OIDURLQueryComponent.m
▸ Compiling OIDFieldMapping.m
▸ Compiling OIDError.m
▸ Compiling OIDAuthorizationRequest.m
▸ Compiling OIDAuthorizationService+IOS.m
▸ Compiling OIDResponseTypes.m
▸ Compiling OIDScopes.m
▸ Compiling OIDServiceDiscovery.m
▸ Building library libAppAuth.a
▸ Build Succeeded
▸ Building AppAuth/AppAuth [Debug]
▸ Check Dependencies
▸ Building AppAuth/AppAuthTests [Debug]
▸ Check Dependencies
▸ Processing UnitTestsInfo.plist
▸ Compiling OIDURLQueryComponentTests.m
▸ Compiling OIDTokenResponseTests.m

❌  /Users/vagrant/git/UnitTests/OIDTokenResponseTests.m:136:35: must specify at least one argument for '...' parameter of variadic macro [-Werror,-Wgnu-zero-variadic-macro-arguments]

  XCTAssertNotNil(response.request);
                                  ^



❌  /Users/vagrant/git/UnitTests/OIDTokenResponseTests.m:137:68: must specify at least one argument for '...' parameter of variadic macro [-Werror,-Wgnu-zero-variadic-macro-arguments]

  XCTAssertEqualObjects(response.accessToken, kAccessTokenTestValue);
        ^



❌  /Users/vagrant/git/UnitTests/OIDTokenResponseTests.m:138:64: must specify at least one argument for '...' parameter of variadic macro [-Werror,-Wgnu-zero-variadic-macro-arguments]

  XCTAssertEqualObjects(response.tokenType, kTokenTypeTestValue);
        ^



❌  /Users/vagrant/git/UnitTests/OIDTokenResponseTests.m:139:60: must specify at least one argument for '...' parameter of variadic macro [-Werror,-Wgnu-zero-variadic-macro-arguments]

  XCTAssertEqualObjects(response.idToken, kIDTokenTestValue);
        ^



❌  /Users/vagrant/git/UnitTests/OIDTokenResponseTests.m:140:70: must specify at least one argument for '...' parameter of variadic macro [-Werror,-Wgnu-zero-variadic-macro-arguments]

  XCTAssertEqualObjects(response.refreshToken, kRefreshTokenTestValue);
        ^



❌  /Users/vagrant/git/UnitTests/OIDTokenResponseTests.m:141:57: must specify at least one argument for '...' parameter of variadic macro [-Werror,-Wgnu-zero-variadic-macro-arguments]

  XCTAssertEqualObjects(response.scope, kScopesTestValue);
        ^



❌  /Users/vagrant/git/UnitTests/OIDTokenResponseTests.m:143:54: must specify at least one argument for '...' parameter of variadic macro [-Werror,-Wgnu-zero-variadic-macro-arguments]

                        kTestAdditionalParameterValue);
        ^



❌  /Users/vagrant/git/UnitTests/OIDTokenResponseTests.m:149:86: must specify at least one argument for '...' parameter of variadic macro [-Werror,-Wgnu-zero-variadic-macro-arguments]

  XCTAssert(expiration > kExpiresInTestValue - 5 && expiration <= kExpiresInTestValue);
        ^



❌  /Users/vagrant/git/UnitTests/OIDTokenResponseTests.m:153:39: must specify at least one argument for '...' parameter of variadic macro [-Werror,-Wgnu-zero-variadic-macro-arguments]

  XCTAssertNotNil(responseCopy.request);
        ^



❌  /Users/vagrant/git/UnitTests/OIDTokenResponseTests.m:154:72: must specify at least one argument for '...' parameter of variadic macro [-Werror,-Wgnu-zero-variadic-macro-arguments]

  XCTAssertEqualObjects(responseCopy.accessToken, kAccessTokenTestValue);
        ^



❌  /Users/vagrant/git/UnitTests/OIDTokenResponseTests.m:155:68: must specify at least one argument for '...' parameter of variadic macro [-Werror,-Wgnu-zero-variadic-macro-arguments]

  XCTAssertEqualObjects(responseCopy.tokenType, kTokenTypeTestValue);
        ^



❌  /Users/vagrant/git/UnitTests/OIDTokenResponseTests.m:156:64: must specify at least one argument for '...' parameter of variadic macro [-Werror,-Wgnu-zero-variadic-macro-arguments]

  XCTAssertEqualObjects(responseCopy.idToken, kIDTokenTestValue);
        ^



❌  /Users/vagrant/git/UnitTests/OIDTokenResponseTests.m:157:74: must specify at least one argument for '...' parameter of variadic macro [-Werror,-Wgnu-zero-variadic-macro-arguments]

  XCTAssertEqualObjects(responseCopy.refreshToken, kRefreshTokenTestValue);
        ^



❌  /Users/vagrant/git/UnitTests/OIDTokenResponseTests.m:158:61: must specify at least one argument for '...' parameter of variadic macro [-Werror,-Wgnu-zero-variadic-macro-arguments]

  XCTAssertEqualObjects(responseCopy.scope, kScopesTestValue);
        ^



❌  /Users/vagrant/git/UnitTests/OIDTokenResponseTests.m:160:54: must specify at least one argument for '...' parameter of variadic macro [-Werror,-Wgnu-zero-variadic-macro-arguments]

                        kTestAdditionalParameterValue);
        ^



❌  /Users/vagrant/git/UnitTests/OIDTokenResponseTests.m:175:39: must specify at least one argument for '...' parameter of variadic macro [-Werror,-Wgnu-zero-variadic-macro-arguments]

  XCTAssertNotNil(responseCopy.request);
        ^



❌  /Users/vagrant/git/UnitTests/OIDTokenResponseTests.m:176:81: must specify at least one argument for '...' parameter of variadic macro [-Werror,-Wgnu-zero-variadic-macro-arguments]

  XCTAssertEqualObjects(responseCopy.request.clientID, response.request.clientID);
        ^



❌  /Users/vagrant/git/UnitTests/OIDTokenResponseTests.m:178:72: must specify at least one argument for '...' parameter of variadic macro [-Werror,-Wgnu-zero-variadic-macro-arguments]

  XCTAssertEqualObjects(responseCopy.accessToken, kAccessTokenTestValue);
        ^



❌  /Users/vagrant/git/UnitTests/OIDTokenResponseTests.m:179:68: must specify at least one argument for '...' parameter of variadic macro [-Werror,-Wgnu-zero-variadic-macro-arguments]

  XCTAssertEqualObjects(responseCopy.tokenType, kTokenTypeTestValue);
        ^



❌  fatal error: too many errors emitted, stopping now [-ferror-limit=]


xcode test exit code: 65
xcode test failed, error: exit status 65
exit status 1
|

@StevenEWright
Copy link
Collaborator Author

Wonderful! Thank you! On it...

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Source/iOS/OIDAuthorizationUICoordinatorIOS.h, line 37 at r1 (raw file):

Previously, WilliamDenniss (William Denniss) wrote… > This slash is a Doxygen macro that will like out to Apple's docs. Did you intend to remove it?
No I did not, in that case!! Thank you!!! Will fix...

Comments from Reviewable

@StevenEWright
Copy link
Collaborator Author

Review status: 10 of 24 files reviewed at latest revision, 1 unresolved discussion.

Source/iOS/OIDAuthorizationUICoordinatorIOS.h, line 37 at r1 (raw file):

Previously, StevenEWright (Steven E Wright) wrote…

No I did not, in that case!! Thank you!!! Will fix...

Done.

Comments from Reviewable

@WilliamDenniss
Copy link
Member

Bitrise shows 1 error:

 Compiling OIDAuthState.m

❌  /Users/vagrant/git/Source/OIDAuthState.m:421:81: must specify at least one argument for '...' parameter of variadic macro [-Werror,-Wgnu-zero-variadic-macro-arguments]

  NSAssert(_pendingActionsSyncObject, @"_pendingActionsSyncObject cannot be nil");

@codecov-io
Copy link

codecov-io commented Nov 29, 2016 •
edited
Loading

Codecov Report

Merging #62 into master will decrease coverage by 0.18%.
The diff coverage is 87.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
- Coverage   76.35%   76.16%   -0.19%     
==========================================
  Files          39       33       -6     
  Lines        2360     1972     -388     
  Branches      123      107      -16     
==========================================
- Hits         1802     1502     -300     
+ Misses        501      426      -75     
+ Partials       57       44      -13
Impacted Files Coverage Δ
UnitTests/OIDScopesTests.m 100% <ø> (ø) ⬆️
Source/OIDAuthState.h 100% <ø> (ø) ⬆️
UnitTests/OIDURLQueryComponentTestsIOS7.m 100% <ø> (ø) ⬆️
UnitTests/OIDGrantTypesTests.m 100% <ø> (ø) ⬆️
Source/OIDTokenRequest.m 61.38% <0%> (ø) ⬆️
Source/OIDServiceDiscovery.m 83.33% <0%> (ø) ⬆️
Source/OIDAuthorizationResponse.m 56.33% <0%> (ø) ⬆️
Source/OIDTokenResponse.m 69.09% <0%> (-1.5%) ⬇️
Source/OIDFieldMapping.m 76.31% <0%> (-0.77%) ⬇️
Source/OIDAuthorizationRequest.m 78.09% <0%> (ø) ⬆️
... and 30 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 579f36a...b3d42d5. Read the comment docs.

@WilliamDenniss WilliamDenniss mentioned this pull request Jun 2, 2018
Merged
@WilliamDenniss
Copy link
Member

#245 is a rebase of this PR with additional fixes.

WilliamDenniss
WilliamDenniss reviewed Jun 2, 2018
View reviewed changes
Source/OIDAuthState.m
@@ -283,7 +282,7 @@ - (BOOL)isAuthorized {
- (void)updateWithAuthorizationResponse:(nullable OIDAuthorizationResponse *)authorizationResponse
error:(nullable NSError *)error {
// If the error is an OAuth authorization error, updates the state. Other errors are ignored.
if (error.domain == OIDOAuthAuthorizationErrorDomain) {
if (error && error.domain == OIDOAuthAuthorizationErrorDomain) {
Copy link
Member

Choose a reason for hiding this comment

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

nil check added here

WilliamDenniss added a commit that referenced this pull request Jun 8, 2018
@WilliamDenniss
Enable pedantic warnings and 'treat warnings as errors'
61e66ed 
Fixes several issues that were out of compliance with these warnings.

Collaboration between Steven E Wright and William Denniss (see #62, #245).
WilliamDenniss pushed a commit that referenced this pull request Jun 8, 2018
@WilliamDenniss
Enable pedantic warnings and 'treat warnings as errors'
0db254b 
Fixes several issues that were out of compliance with these warnings.

Collaboration between Steven E Wright and William Denniss (see #62, #245).
WilliamDenniss pushed a commit that referenced this pull request Jun 8, 2018
@StevenEWright @WilliamDenniss
Enable pedantic warnings and 'treat warnings as errors'
c4a6277 
Fixes several issues that were out of compliance with these warnings.

Collaboration between Steven E Wright and William Denniss (see #62, #245).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WilliamDenniss WilliamDenniss WilliamDenniss left review comments

Assignees
No one assigned
Labels
openid-cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@StevenEWright @WilliamDenniss @codecov-io