-
Notifications
You must be signed in to change notification settings - Fork 0
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
Update README files #69
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #69 +/- ##
=========================================
Coverage 62.40% 62.40%
Complexity 821 821
=========================================
Files 119 119
Lines 3809 3809
Branches 512 512
=========================================
Hits 2377 2377
Misses 1228 1228
Partials 204 204 |
@armiol @alexander-yevsyukov PTAL And let's discuss next steps after review of this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yevhenii-nadtochii Mostly LGTM. See my comments though.
Also, please gather us with @alexander-yevsyukov to discuss the points you have raised in the PR description.
logging-backend/README.md
Outdated
The following services may be injected: | ||
|
||
| Service | Default | | ||
|-----------------------|---| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's format the table borders to fit the right edge here and in the following lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDEA Markdown plugin can do it automatically.
|
||
This module contains Flogger [sources][flogger-github] built with Gradle. | ||
This directory contains Flogger [modules][flogger-github] built with Gradle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to speak about repackaging we performed somewhere. This file seems to be a proper place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comments.
README.md
Outdated
the current request. | ||
A logging context refers to a set of attributes that are attached to all log | ||
records while a context is installed. For instance, you can attach a user ID | ||
to all log records for the current request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last sentence probably deserves a code snippet. But I don't insist at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logging-fake-backend/README.md
Outdated
check(logged[0].literalArgument == message) | ||
``` | ||
|
||
To use it, add this backend to `implementation` configuration instead of `runtimeOnly`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it testImplementation
, unless we do test fixtures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway a code example would be helpful here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yevhenii-nadtochii LGTM with a minor suggestion.
README.md
Outdated
```kotlin | ||
val handler = RequestHandler() | ||
with(handler) { | ||
handle(Request(action = "create", user = "Maks")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Max"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR updates README files.
The following modules have been updated:
tests/logging-smoke-test
logging-fake-backend
logging-context
logging-backend
logging
flogger
I've renamed
Module.md
toREADME.md
becauseREADME
files are rendered by GitHub even if they are in nested directories.Possible actions:
slf4j
backend, and after repackaging we are incompatible with Flogger's one. We should either create an issue to add this backend or remove two test modules that use this backend.io.spine.testing.logging
package intests/fixtures
module. And we also haveio.spine.logging.testing
intestutil-logging
. Earlier, we decided to go with the latter option.logging-fake-backend
tobackends/jvm-dynamic-backend
. We don't need "logging" prefix because this module is not published. Also, I'm not sure whether to extractcaptureLogData()
to a separate module. It feels so, but I haven't come up with a module name.SimpleLoggerBackend
. We have our own default backend (StdLoggerBackend
), so Flogger's one can be removed.logging
prefix in module names? We use it for published modules, and it starts to pollute modules hierarchy with long names. MaybespinePublihsing{}
can do it for us.Flogger
(Remove deprecatedLogging
interface #62). Putting ofspine-logging
on the classpath doesn't lead to the console output anymore. It leads to an error. We also have to putlogging-backend
explicitly.