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

Logger constants refactored, format argument added, better formatting of failed (non) equality assertions #9315

Merged
merged 26 commits into from Jun 4, 2023

Conversation

skelly37
Copy link
Contributor

@skelly37 skelly37 commented May 29, 2023

Description

I have (hopefully) simplified the log.nu internal structure and added customizable log format for all log commands

User-Facing Changes

  • Replaced constants with env records for:
    • ansi (newly added)
    • log level
    • prefix
    • short prefix
  • Added format argument to all log commands
  • Assertions for (not) equality (equal, not equal, greater, lesser...) now put left and right values inside ' quotes, so the assertions for strings are more meaningful
  • Documented the %-formatting of log messages

Tests + Formatting

After Submitting

@skelly37
Copy link
Contributor Author

skelly37 commented May 29, 2023

I think that lib.rs or mod.nu does some nasty stuff to the env, so now the log.nu is not parsed correctly. The feature seems to work correctly. Could anyone help me?

image

@skelly37 skelly37 changed the title Logger constants refactored, format argument added [help needed] Logger constants refactored, format argument added May 29, 2023
crates/nu-std/std/mod.nu Outdated Show resolved Hide resolved
@skelly37
Copy link
Contributor Author

skelly37 commented May 29, 2023

TODO:

  • Tests for correctness of default env values (e.g. $env.LOG_LEVEL.CRITICAL == 50)
  • Tests proving that log * commands can be affected by modifying env (e.g. modifying $env.LOG_FORMAT affects the output)
  • Tests proving correctness of the --format flag
  • Tests for the updated log custom
  • Documentation for the %-formatting of logs (e.g. %MSG%)
  • Update annotations for --help
  • Probably do something about mod.nu, lib.rs and env_exporter.nu workaround
  • Think about some unification possibilites regarding exported log custom & internal log-formatted
  • Self-review
  • Remove draft 😅

@amtoine amtoine added the std-library Defining and improving the standard library written in nu and the core Rust ccommands label May 30, 2023
Copy link
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

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

i did not have a close look at everything:

  • the env_reporter.nu file
  • the log.nu module
  • the tests

wanted to make sure this was ok before moving too far in the PR 😉

crates/nu-std/std/mod.nu Outdated Show resolved Hide resolved
@amtoine
Copy link
Member

amtoine commented May 31, 2023

@skelly37
this looks better to me so i'll let you finish the PR 😌

try to keep it not too big if possible, to make the review as easy as possible 🤞

@skelly37
Copy link
Contributor Author

@skelly37
this looks better to me so i'll let you finish the PR 😌

try to keep it not too big if possible, to make the review as easy as possible 🤞

Sure, you see the TODO

Only last test case and self-review are left :)

@amtoine
Copy link
Member

amtoine commented May 31, 2023

Sure, you see the TODO

Only last test case and self-review are left :)

ohhh yes, great 😊

@skelly37 skelly37 marked this pull request as ready for review June 1, 2023 14:14
@skelly37 skelly37 requested a review from amtoine June 1, 2023 14:15
@skelly37
Copy link
Contributor Author

skelly37 commented Jun 1, 2023

@amtoine I think it's ready for review. I suggest reading log.nu in IDE instead of github diff, as it was deeply refactored and needs reading it as a whole :)

@skelly37 skelly37 changed the title [help needed] Logger constants refactored, format argument added Logger constants refactored, format argument added, better formatting of failed (not) equality assertions Jun 1, 2023
@skelly37 skelly37 changed the title Logger constants refactored, format argument added, better formatting of failed (not) equality assertions Logger constants refactored, format argument added, better formatting of failed (non) equality assertions Jun 1, 2023
@amtoine
Copy link
Member

amtoine commented Jun 1, 2023

doing it right now... ♻️

Copy link
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

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

coooool, thanks for this quite big refactoring, i think it looks better and i like the use of env variables better ✨

a few remarks

  • a thread about the errors in std log custom
  • in the tests, could we have a single now, format-message and run-command instead of duplicating them across test modules?
  • a thread about logger_env_test_config.nu

we're close 🤞

crates/nu-std/tests/logger_tests/test_log_custom.nu Outdated Show resolved Hide resolved
crates/nu-std/std/log.nu Outdated Show resolved Hide resolved
@skelly37 skelly37 requested a review from amtoine June 1, 2023 17:40
@skelly37
Copy link
Contributor Author

skelly37 commented Jun 1, 2023

Regarding the code duplication: I have moved now and format-message to a separate file, but I think that the command runners differ too much to actually benefit from moving them to one place.

Copy link
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

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

Regarding the code duplication: I have moved now and format-message to a separate file, but I think that the command runners differ too much to actually benefit from moving them to one place.

great that looks better 😌

i've refactored the error a tiny bit

  • it's in a command so that all the same errors look the same
  • it looks like the following now, instead of having all the values and tips on the same "error" line at the top
> use std
> std log custom "msg" "%MSG%" 21
Error:   × Cannot deduce level prefix for given log level: 21.
   ╭─[entry #2:1:1]
 1 │ std log custom "msg" "%MSG%" 21
   ·                              ─┬
   ·                               ╰── Invalid log level.
        Available log levels in $env.LOG_LEVEL:
            CRITICAL: 50
            ERROR: 40
            WARNING: 30
            INFO: 20
            DEBUG: 10

   ╰────

@amtoine
Copy link
Member

amtoine commented Jun 3, 2023

woopsie, my bad, that's a dumb error => working on it... ♻️

@skelly37
Copy link
Contributor Author

skelly37 commented Jun 3, 2023

Thanks for the patch & review, @amtoine!

With my last commit I think we're ready, aren't we? 😋

@skelly37 skelly37 requested a review from amtoine June 3, 2023 13:26
Copy link
Member

@amtoine amtoine 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 the patch & review, @amtoine!

anytime 😌

With my last commit I think we're ready, aren't we? yum

yeah, i think this is in good shape 😋

let's land this 🥳

@amtoine amtoine merged commit df15fc2 into nushell:main Jun 4, 2023
16 checks passed
@skelly37 skelly37 deleted the refactor/logger branch June 4, 2023 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
std-library Defining and improving the standard library written in nu and the core Rust ccommands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants