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

feat(spec): Added designated initializer to BugsnagConfiguration #446

Merged

Conversation

robinmacharg
Copy link
Contributor

Goal

The notifier spec. states that an apiKey must be provided when initializing a BugsnagConfiguration. To this end removing the default constructor and providing a new apiKey-accepting designated initializer is the solution.

Design

An initializer has been added to BugsnagConfiguration and its designated status indicated with the NS_DESIGNATED_INITIALIZER macro. init() is required (calls to designated initializers MUST call super) but has been made to fail if called. It was desired to provide apiKey as an unnamed parameter but the NS_SWIFT_NAME macro doesn't appear to offer this functionality.

Changeset

A new initializer was added. The default init() was made to fail.

Tests

Unit tests were added to ensure that the new initializer performed as expected. All tests that depended on the default init() have been modified to use the new initializer. A Swift test was added to ensure that the method is exposed correctly.

Review

Outstanding Questions

  • This pull request is ready for:
    • Initial review of the intended approach, not yet feature complete
    • Structural review of the classes, functions, and properties modified
    • Final review
  • The correct target branch has been selected (master for fixes, next for
    features)
  • Consistency across platforms for structures or concepts added or modified
  • Consistency between the changeset and the goal stated above
  • Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
  • Usage friction - is the proposed change in usage cumbersome or complicated?
  • Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
  • Concurrency concerns - if components are accessed asynchronously, what issues will arise
  • Thoroughness of added tests and any missing edge cases
  • Idiomatic use of the language

@robinmacharg robinmacharg requested a review from kattrali January 22, 2020 15:25
@robinmacharg robinmacharg marked this pull request as ready for review January 22, 2020 15:29
@kattrali kattrali changed the base branch from master to spec-compliance January 22, 2020 15:29
Copy link
Contributor

@kattrali kattrali left a comment

Choose a reason for hiding this comment

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

Haven't finished going through everything, but I added a few initial thoughts. In addition, I had one which didn't cleanly apply to the current diff:

  • The declaration for Configuration.apiKey is marked as nullable where it should now be nonnull.

.gitignore Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Source/BugsnagConfiguration.h Outdated Show resolved Hide resolved
Source/BugsnagConfiguration.h Outdated Show resolved Hide resolved
Copy link
Contributor

@kattrali kattrali left a comment

Choose a reason for hiding this comment

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

Looks good and overall works as expected. I have one interface concern particularly for Swift that I've added inline.

Source/BugsnagConfiguration.m Outdated Show resolved Hide resolved
Copy link
Contributor

@kattrali kattrali left a comment

Choose a reason for hiding this comment

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

The implementation looks good! Added a couple comments for future proofing against accidental regressions in the tests.

Tests/BugsnagSwiftConfigurationTests.swift Outdated Show resolved Hide resolved
Tests/BugsnagSwiftConfigurationTests.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@fractalwrench fractalwrench left a comment

Choose a reason for hiding this comment

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

Looks like a nice changeset 🎉

Happy to approve on the basis that this will be rebased + tests will pass on CI before merging.

@robinmacharg robinmacharg force-pushed the add-configuration-designated-initializer branch from ac94f68 to 1d43f5d Compare February 4, 2020 09:13
robinmacharg and others added 4 commits February 4, 2020 09:33
(effectively) removed convenience init, updated ObjC tests, added Swift test, adjusted designated initializer's Swift exposure
fix: Updated .gitignore to exclude IDEWorkspaceChecks.plist
Adding swift has broken the Xcode 10.2 env, fix by creating a dir
https://stackoverflow.com/questions/55389080/
@robinmacharg robinmacharg merged commit 6c74bfb into spec-compliance Feb 4, 2020
@robinmacharg robinmacharg deleted the add-configuration-designated-initializer branch February 4, 2020 14:15
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