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: Consolidate internal logging configuration #472

Conversation

robinmacharg
Copy link
Contributor

Goal

Consolidate internal logging configuration to only use the BSG_LOG_LEVEL preprocessor macro.

Design

The smallest change possible: an import and a variable change. Additional first-pass documentation.

Changeset

BSG_KSLogger.h
BugsnagLogger.h

Tests

Manual testing with the example apps, repeated running of pod install. How to automate testing in an appropriate amount of time was not obvious.

Review

Outstanding Questions

  • Platform Documentation could do with being improved (but this can be done as a separate task; raised internally)
  • Additional docs (e.g. Carthage, Manual) are probably required for completeness (ditto)
  • While this has been tested to a reasonable extent, the #ifdef macros are a little soupy; are there any logging edge cases I've missed?
  • 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 force-pushed the robinmacharg/Unify-KSCrash-logging-into-Bugsnag-logging branch from 5613298 to 8badd93 Compare February 27, 2020 10:21
Source/BugsnagLogger.h Outdated Show resolved Hide resolved
Source/BugsnagLogger.h Show resolved Hide resolved
…g-into-Bugsnag-logging

# Conflicts:
#	CHANGELOG.md
@robinmacharg robinmacharg self-assigned this Feb 27, 2020
@kattrali kattrali changed the title (feat) Consolidate internal logging configuration feat: Consolidate internal logging configuration Feb 27, 2020
@kattrali kattrali merged commit 27ba1e3 into spec-compliance Feb 27, 2020
@kattrali kattrali deleted the robinmacharg/Unify-KSCrash-logging-into-Bugsnag-logging branch February 27, 2020 11:58
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