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

Alter api key validation to log warning #492

Merged
merged 3 commits into from
Mar 24, 2020
Merged

Conversation

fractalwrench
Copy link
Contributor

Goal

Alters api key validation so that an error is logged and configuration is populated as normal.

This has the side effect of allowing the NSError parameter to be removed from initWithApiKey as it is no longer populated in any codepath.

Changeset

  • Updated initWithApiKey: signature by removing NSError parameter
  • Updated initWithApiKey to log an error and continue construction rather than returning nil
  • Removed constants used for populating the NSError

Tests

Relied on existing test coverage which was updated to use new method signatures.

Copy link
Contributor

@tomlongridge tomlongridge left a comment

Choose a reason for hiding this comment

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

LGTM as long as we're confident in the compile-time null check for which I'd like @robinmacharg's input.

@fractalwrench fractalwrench force-pushed the v6-api-key-validation branch from 829977b to 8a91e17 Compare March 20, 2020 09:59
@tomlongridge tomlongridge self-requested a review March 20, 2020 16:00
Copy link
Contributor

@tomlongridge tomlongridge left a comment

Choose a reason for hiding this comment

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

Actually, from discussion on the Android/JS changes: we should be throwing an error if the API key is empty so unfortunately I think this means we need to reinstate the error parameter...

@tomlongridge tomlongridge self-requested a review March 20, 2020 16:28
@tomlongridge
Copy link
Contributor

Actually, from discussion on the Android/JS changes: we should be throwing an error if the API key is empty so unfortunately I think this means we need to reinstate the error parameter...

Discussed IRL and agreed with @fractalwrench that the proposed changes are probably the more preferable - either way we have a slight spec deviation, but with this implementation it doesn't affect the signature.

@fractalwrench
Copy link
Contributor Author

Fixed a compile issue with the mazerunner scenarios and verified by running the breadcrumb scenario manually.

@robinmacharg
Copy link
Contributor

LGTM as long as we're confident in the compile-time null check for which I'd like @robinmacharg's input.

From Apples dev blog: "The compiler will tell you if you try to break the rules."

Copy link
Contributor

@robinmacharg robinmacharg left a comment

Choose a reason for hiding this comment

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

LGTM.

@fractalwrench fractalwrench merged commit 0c50d80 into v6 Mar 24, 2020
@fractalwrench fractalwrench deleted the v6-api-key-validation branch March 24, 2020 16:26
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.

3 participants