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

Change default logger level to ERROR; Migrate publishing script to SDK registry #65

Merged
merged 3 commits into from
Dec 9, 2021

Conversation

DzmitryFomchyn
Copy link
Contributor

@DzmitryFomchyn DzmitryFomchyn force-pushed the df-logger-level branch 2 times, most recently from ba354e4 to be6993a Compare November 25, 2021 13:14
@DzmitryFomchyn DzmitryFomchyn force-pushed the df-logger-level branch 4 times, most recently from def075a to 76ac330 Compare December 2, 2021 18:25
@DzmitryFomchyn DzmitryFomchyn marked this pull request as ready for review December 2, 2021 18:27
Copy link

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

Thanks a lot for refreshing the publishing scripts! We'll need to update the readme as well to mention the need to use a downloads access token.

common/gradle.properties Show resolved Hide resolved
@@ -25,7 +25,7 @@ object MapboxLogger : Logger {
*/
@LogLevel
@Volatile
var logLevel: Int = VERBOSE
var logLevel: Int = NONE

Choose a reason for hiding this comment

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

Wondering if this should happen here, or rather be changed in the Common SDK and controlled via com.mapbox.common.LogConfiguration#setFilterLevel which developers have access to. All our logs, even from the platform SDKs, should ideally be wired through the Common SDK.

Normally, developers shouldn't interact with this logger instance directly and var logLevel is not part of the Logger interface anyway.

Choose a reason for hiding this comment

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

Common SDK targeting Android should automatically set the CommonSingletonModuleProvider.loggerInstance as the logger backend which I'm not sure if it does already.

Copy link
Member

Choose a reason for hiding this comment

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

Also, imo, we should also keep logging errors

Copy link
Contributor Author

@DzmitryFomchyn DzmitryFomchyn Dec 7, 2021

Choose a reason for hiding this comment

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

Common SDK targeting Android should automatically set the CommonSingletonModuleProvider.loggerInstance as the logger backend which I'm not sure if it does already.

You are right, Common SDK doesn't use base Logger. I've opened a follow-up issue to fix that https://github.com/mapbox/mapbox-sdk-common/issues/2346. We've discussed this with Lukas in DM and decided that we should fix default logging level in this repo and also, logging in the Common SDK.

Also, imo, we should also keep logging errors

Agree, we can leave errors logging by default. I've changed log level to ERROR.

@DzmitryFomchyn DzmitryFomchyn changed the title Change default logger level to NONE; Migrate publishing script to SDK registry Change default logger level to ERROR; Migrate publishing script to SDK registry Dec 7, 2021
@DzmitryFomchyn
Copy link
Contributor Author

Thanks a lot for refreshing the publishing scripts! We'll need to update the readme as well to mention the need to use a downloads access token.

Good catch, thanks Lukas, I've added a README note about credentials.

Copy link
Member

@tobrun tobrun left a comment

Choose a reason for hiding this comment

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

tremendous effort for a 1 line change :D

Copy link

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

:shipit:

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