Add Logger module #21
Conversation
|
@LukasPaczos ptal, and let me know if you'll have comments\another vision on it. Cause I doubt about MapboxNavigationModuleType#Logger and where Logger class should be placed (in navigation project it placed separately from implementation). |
There was a problem hiding this comment.
Looks good but the implementation should be moved to https://github.com/mapbox/mapbox-base-android/tree/master/common/src/main/java/com/mapbox/common/module and the module type should be in https://github.com/mapbox/mapbox-base-android/blob/master/annotations/src/main/java/com/mapbox/annotation/module/MapboxModuleType.kt.
|
We'll also need to initialize the Bintray repository. |
|
Stopped for now due to other priorities |
annotations/src/main/java/com/mapbox/annotation/module/MapboxModuleType.kt
Outdated
Show resolved
Hide resolved
|
The Bintray repo is available in https://bintray.com/mapbox/mapbox/com.mapbox.common%3Alogger. @Lebedinsky whenever you have comments fixed, we can pair and push the version to be able to request the artifactory snapshot repo. |
Lebedinsky
force-pushed
the
rl-add-logger
branch
from
103afdd to
6c4cbab
Compare
liblogger/src/main/java/com/mapbox/logger/annotations/LogLevel.kt
Outdated
Show resolved
Hide resolved
| * | ||
| * @param message The message you would like logged. | ||
| */ | ||
| data class Message(val message: String) |
There was a problem hiding this comment.
maybe use inline classes?
There was a problem hiding this comment.
@LukasPaczos @Guardiola31337 Are we ok with inline classes? They are still experimental...
There was a problem hiding this comment.
I'd prefer avoiding any experimental APIs as this can mean that down the road bumping the Kotlin compiler on the client-side might break the binary compatibility with the SDK.
There was a problem hiding this comment.
Yeah I agree with @LukasPaczos let's try to avoid them if possible.
| * @param tag used to identify the source of a log message. It usually identifies | ||
| * the class or activity where the log call occurs. | ||
| */ | ||
| data class Tag(val tag: String) |
| import android.util.Log | ||
| import com.mapbox.common.logger.model.Message | ||
| import com.mapbox.common.logger.model.Tag | ||
| import io.mockk.* |
There was a problem hiding this comment.
Do you have rule for skipping such check in analyzers (ktlint, etc)?
There was a problem hiding this comment.
What do you mean? The Wildcard import rule? If so, it doesn't seem it's set in this project.
|
|
||
| @Test | ||
| fun verboseTestIncorrectLogLevel() { | ||
| val throwable = mockk<Throwable>() |
There was a problem hiding this comment.
better to use parametrized tests here
https://github.com/junit-team/junit4/wiki/Parameterized-tests
There was a problem hiding this comment.
@Guardiola31337 wdyt?
There was a problem hiding this comment.
Parameterized tests work pretty well if you want to test multiple inputs and verify their respective expected outputs and in this simple case I don't see how Parameterized / @Parameters would make this test better. How were you envisioning using parameterized tests here @alexander-kulikovskii?
|
Thanks for pushing the artifacts @Lebedinsky. I've requested JCenter and Artifactory access, but currently, it's throwing: but obviously the package isn't empty. I'm going to give it a shot tomorrow morning again. |
|
Still running into the same issue. I do not want to release the package just to request JCenter and Artifactory access, so I wrote a support ticket. |
LukasPaczos
force-pushed
the
rl-add-logger
branch
from
c8110cf to
8e41d92
Compare
|
I just rebased the PR and tried requesting JCenter access again and failed with this message again: Are we good to merge besides that @Lebedinsky? If yes, let's do that and release it to Bintray only. When the package will have a version published, maybe then it'll allow me to request JCenter and Artifactory access. |
LukasPaczos
force-pushed
the
rl-add-logger
branch
from
8e41d92 to
18bb176
Compare
|
After releasing the From now on, the snapshots are being hosted at http://oss.jfrog.org/oss-snapshot-local/com/mapbox/common/logger/. |
This PR is about moving Logger from navigation SDK to base-android repo as part of this task