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.

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

feat: json logger #252

Merged
merged 1 commit into from
Apr 28, 2023
Merged

feat: json logger #252

merged 1 commit into from
Apr 28, 2023

Conversation

berendsliedrecht
Copy link
Member

@berendsliedrecht berendsliedrecht commented Mar 10, 2023 •
edited
Loading

closes #164
closes #242

@codecov
Copy link

codecov bot commented Mar 31, 2023 •
edited
Loading

Codecov Report

Merging #252 (df539e5) into main (d8b6128) will decrease coverage by 0.2%.
The diff coverage is 0.0%.

❗ Current head df539e5 differs from pull request most recent head ac2356f. Consider uploading reports for the commit ac2356f to get more accurate results

Impacted Files Coverage Δ
crates/afj-rest/src/cloudagent/connection.rs 0.0% <0.0%> (ø)
crates/afj-rest/src/web.rs 0.0% <0.0%> (ø)
crates/agent/src/error.rs 0.0% <0.0%> (ø)
...ns/src/automations/create_credential_definition.rs 0.0% <0.0%> (ø)
...es/automations/src/automations/credential_offer.rs 0.0% <0.0%> (ø)
crates/cli/src/cli.rs 0.0% <ø> (ø)
crates/cli/src/main.rs 12.5% <0.0%> (-1.8%) ⬇️
crates/cli/src/modules/automation.rs 0.0% <0.0%> (ø)
crates/cli/src/modules/basic_message.rs 0.0% <0.0%> (ø)
crates/cli/src/modules/configuration.rs 0.0% <0.0%> (ø)
... and 14 more

... and 3 files with indirect coverage changes

morrieinmaas
morrieinmaas reviewed Mar 31, 2023
View reviewed changes
crates/logger/src/lib.rs Outdated
pub struct LoggerState {
/// Whether the logger is already initialized
pub init: bool,

/// Whether the output that is being logged should also be copied
pub should_copy: bool,
pub should_final_copy: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe should_copy_clip?

Copy link
Member Author

Choose a reason for hiding this comment

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

As in "should copy to clipboard?"

I added final here to indicate that the final log, i.e. the value you would like to copy (invite, connection id, etc.) would be copied.

I don't think that should_copy_clipboard or should_final_copy are good. I will have a think.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mena it's not world changing especially from a user perspective. But yeah let's name it sth more eaningful then should_final_copy - found that a bit confusing. Otherwise really great changes

@berendsliedrecht berendsliedrecht force-pushed the json-logging branch 2 times, most recently from 83e4775 to 6947565 Compare March 31, 2023 09:04
morrieinmaas
morrieinmaas approved these changes Mar 31, 2023
View reviewed changes
Copy link
Contributor

@morrieinmaas morrieinmaas left a comment

Choose a reason for hiding this comment

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

Overall looks great

crates/cli/src/modules/webhook.rs Show resolved Hide resolved
crates/logger/src/lib.rs Outdated
pub struct LoggerState {
/// Whether the logger is already initialized
pub init: bool,

/// Whether the output that is being logged should also be copied
pub should_copy: bool,
pub should_final_copy: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

I mena it's not world changing especially from a user perspective. But yeah let's name it sth more eaningful then should_final_copy - found that a bit confusing. Otherwise really great changes

crates/logger/src/macros.rs Show resolved Hide resolved
crates/cli/src/cli.rs Outdated Show resolved Hide resolved
@berendsliedrecht berendsliedrecht force-pushed the json-logging branch 3 times, most recently from 909b845 to 7d06be3 Compare March 31, 2023 09:30
morrieinmaas
morrieinmaas suggested changes Mar 31, 2023
View reviewed changes
Copy link
Contributor

@morrieinmaas morrieinmaas 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 more Qs/nit picks

@berendsliedrecht
Copy link
Member Author

Fixed your comments. can I merged it?

@berendsliedrecht berendsliedrecht force-pushed the json-logging branch 5 times, most recently from 51d7d8b to ee6ed2c Compare April 6, 2023 08:19
morrieinmaas
morrieinmaas approved these changes Apr 6, 2023
View reviewed changes
Copy link
Contributor

@morrieinmaas morrieinmaas left a comment

Choose a reason for hiding this comment

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

LGTM, @blu3beri !

@morrieinmaas
Copy link
Contributor

LGTM, @blu3beri !

There's some failing tests however...

@berendsliedrecht berendsliedrecht force-pushed the json-logging branch 6 times, most recently from 3aacd61 to 204a1ff Compare April 28, 2023 11:32
@berendsliedrecht
feat: json logger
ac2356f 
Signed-off-by: blu3beri <[email protected]>
@berendsliedrecht berendsliedrecht merged commit 9e9b52a into animo:main Apr 28, 2023
@berendsliedrecht berendsliedrecht deleted the json-logging branch April 28, 2023 11:43
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@morrieinmaas morrieinmaas morrieinmaas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unify json and normal logger Allow json output format
2 participants
@berendsliedrecht @morrieinmaas