Skip to content

Inject logger into sdk modules #2774

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
Merged
Guardiola31337 merged 1 commit into from
Apr 21, 2020
Merged

Inject logger into sdk modules #2774

Guardiola31337 merged 1 commit into from
Apr 21, 2020

Conversation

@Lebedinsky
Copy link
Contributor

@Lebedinsky Lebedinsky commented Apr 14, 2020 •
edited
Loading

Description

Injection Logger into each module of Navigation SDK. Part of tailwork for logger module implementation issue

Didn't inject into telemetry (in progress as I know, so better inject logger to it after finish this work) and UI modules (also in progress as I know)

  • I have added any issue links
  • I have added all related labels (bug, feature, new API(s), SEMVER, etc.)
  • I have added the appropriate milestone and project boards

Testing

  • I have tested locally (including SNAPSHOT upstream dependencies if needed) through testapp/demo app and run all activities to avoid regressions
  • I have tested via a test drive, or a simulation/mock location app
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have updated the CHANGELOG including this PR

@Lebedinsky Lebedinsky added this to the v1.0.0 milestone Apr 14, 2020
@Lebedinsky Lebedinsky self-assigned this Apr 14, 2020
@Lebedinsky Lebedinsky force-pushed the rl-186-inject-logger branch 2 times, most recently from 4e67c0a to 98aaa20 Compare April 14, 2020 22:15
LukasPaczos
LukasPaczos reviewed Apr 15, 2020
View reviewed changes
@Lebedinsky Lebedinsky force-pushed the rl-186-inject-logger branch from 026525d to 938f6b0 Compare April 17, 2020 02:17
@Guardiola31337
Copy link
Contributor

Guardiola31337 commented Apr 17, 2020

Dokka validation in action 👀

No documentation for com.******.navigation.route.onboard.MapboxOnboardRouter$logger (MapboxOnboardRouter.kt:40)
No documentation for com.******.navigation.core.replay.history.ReplayHistoryPlayer$logger (ReplayHistoryPlayer.kt:26)
No documentation for com.******.navigation.core.replay.history.ReplayHistoryMapper$logger (ReplayHistoryMapper.kt:21)
No documentation for com.******.navigation.core.trip.service.MapboxTripService$logger (MapboxTripService.kt:30)
No documentation for com.******.navigation.core.trip.session.MapboxTripSession$logger (MapboxTripSession.kt:54)

Great work @RingerJK 🙇

@Lebedinsky Lebedinsky force-pushed the rl-186-inject-logger branch from 3c78a59 to e9cea50 Compare April 17, 2020 20:54
Guardiola31337
Guardiola31337 reviewed Apr 17, 2020
View reviewed changes
Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

Left some comments / questions @Lebedinsky

@Lebedinsky Lebedinsky force-pushed the rl-186-inject-logger branch from 9e13985 to d624897 Compare April 20, 2020 15:25
Guardiola31337
Guardiola31337 reviewed Apr 20, 2020
View reviewed changes
Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

We're missing the Logger in MapboxNavigationTelemetry and TelemetryLocationAndProgressDispatcher @Lebedinsky

@kmadsen kmadsen mentioned this pull request Apr 21, 2020
Merged
10 tasks
@Lebedinsky Lebedinsky force-pushed the rl-186-inject-logger branch 2 times, most recently from 909d7dc to 17943cb Compare April 21, 2020 15:20
Guardiola31337
Guardiola31337 reviewed Apr 21, 2020
View reviewed changes
@Lebedinsky Lebedinsky force-pushed the rl-186-inject-logger branch from b0cbc19 to f626846 Compare April 21, 2020 22:33
Guardiola31337
Guardiola31337 reviewed Apr 21, 2020
View reviewed changes
@Lebedinsky Lebedinsky force-pushed the rl-186-inject-logger branch from ce61143 to 465b674 Compare April 21, 2020 23:35
@Lebedinsky Lebedinsky force-pushed the rl-186-inject-logger branch from adc6790 to 8f73371 Compare April 21, 2020 23:39
Guardiola31337
Guardiola31337 approved these changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the feedback @Lebedinsky 🚀 🚀

libdirections-onboard/src/main/java/com/mapbox/navigation/route/onboard/MapboxOnboardRouter.kt
private val navigatorNative: MapboxNativeNavigator,
config: MapboxOnboardRouterConfig
config: MapboxOnboardRouterConfig,
private val logger: Logger
Copy link
Contributor

@Guardiola31337 Guardiola31337 Apr 21, 2020

Choose a reason for hiding this comment

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

For documentation purposes noting that this breaks SemVer cc @langsmith

...vigation-core/src/main/java/com/mapbox/navigation/core/replay/history/ReplayHistoryMapper.kt
private val gson: Gson = Gson(),
private val customEventMapper: CustomEventMapper? = null
private val customEventMapper: CustomEventMapper? = null,
private val logger: Logger
Copy link
Contributor

@Guardiola31337 Guardiola31337 Apr 21, 2020

Choose a reason for hiding this comment

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

For documentation purposes noting that this breaks SemVer cc @langsmith

...vigation-core/src/main/java/com/mapbox/navigation/core/replay/history/ReplayHistoryPlayer.kt
*/
class ReplayHistoryPlayer {
class ReplayHistoryPlayer(
logger: Logger
Copy link
Contributor

@Guardiola31337 Guardiola31337 Apr 21, 2020

Choose a reason for hiding this comment

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

For documentation purposes noting that this breaks SemVer cc @langsmith

libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/service/MapboxTripService.kt
private val initializeLambda: () -> Unit,
private val terminateLambda: () -> Unit
private val terminateLambda: () -> Unit,
private val logger: Logger
Copy link
Contributor

@Guardiola31337 Guardiola31337 Apr 21, 2020

Choose a reason for hiding this comment

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

For documentation purposes noting that this breaks SemVer cc @langsmith

libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/service/MapboxTripService.kt
constructor(
applicationContext: Context,
tripNotification: TripNotification,
logger: Logger
Copy link
Contributor

@Guardiola31337 Guardiola31337 Apr 21, 2020

Choose a reason for hiding this comment

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

For documentation purposes noting that this breaks SemVer cc @langsmith

libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/session/MapboxTripSession.kt
private val navigator: MapboxNativeNavigator = MapboxNativeNavigatorImpl,
threadController: ThreadController = ThreadController
threadController: ThreadController = ThreadController,
private val logger: Logger
Copy link
Contributor

@Guardiola31337 Guardiola31337 Apr 21, 2020

Choose a reason for hiding this comment

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

For documentation purposes noting that this breaks SemVer cc @langsmith

@Guardiola31337 Guardiola31337 added Core Work related to core navigation and integrations. backwards incompatible Requires a SEMVER major version change. and removed navigation-core labels Apr 21, 2020
@codecov-io
Copy link

codecov-io commented Apr 21, 2020

Codecov Report

Merging #2774 into master will increase coverage by 0.00%.
The diff coverage is 39.62%.

@@            Coverage Diff            @@
##             master    #2774   +/-   ##
=========================================
  Coverage     34.96%   34.96%           
  Complexity     2102     2102           
=========================================
  Files           541      541           
  Lines         19367    19387   +20     
  Branches       1832     1833    +1     
=========================================
+ Hits           6771     6778    +7     
- Misses        11785    11798   +13     
  Partials        811      811

@Guardiola31337
Copy link
Contributor

Guardiola31337 commented Apr 21, 2020

Going ahead and merging here. Great work @Lebedinsky

@Guardiola31337 Guardiola31337 merged commit 869bb29 into master Apr 21, 2020
@Guardiola31337 Guardiola31337 deleted the rl-186-inject-logger branch April 21, 2020 23:54
@Guardiola31337 Guardiola31337 mentioned this pull request Apr 22, 2020
Merged
9 tasks
Guardiola31337
Guardiola31337 reviewed Apr 22, 2020
View reviewed changes
libnavigator/build.gradle

dependencies {
implementation(project(':libnavigation-base'))
implementation dependenciesList.mapboxCommon
Copy link
Contributor

@Guardiola31337 Guardiola31337 Apr 22, 2020

Choose a reason for hiding this comment

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

Why this dependency was added 🤔? Do you remember @Lebedinsky? I've removed it and didn't run into any issues cc @LukasPaczos

Copy link
Contributor

@Guardiola31337 Guardiola31337 Apr 22, 2020

Choose a reason for hiding this comment

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

Refs. #2774 (comment)

Copy link
Contributor Author

@Lebedinsky Lebedinsky Apr 22, 2020

Choose a reason for hiding this comment

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

We can remove it
I added it before Sensor classes was refactored by Kyle, and it was needed…. but after Kyle’s refactoring it is useless anymore. Great catch, Pablo 👍

This was referenced Apr 22, 2020
Merged
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

4 more reviewers

@Guardiola31337 Guardiola31337 Guardiola31337 approved these changes

@RingerJK RingerJK RingerJK left review comments

@LukasPaczos LukasPaczos LukasPaczos left review comments

@kmadsen kmadsen kmadsen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Lebedinsky Lebedinsky

Labels

backwards incompatible Requires a SEMVER major version change. Core Work related to core navigation and integrations.

Projects

None yet

Milestone

v1.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@Lebedinsky @Guardiola31337 @codecov-io @kmadsen @RingerJK @LukasPaczos