Few issues solved - separator/DRY/list(...) by Polkas · Pull Request #96 · daroczig/logger

Polkas · 2021-11-08T22:48:53Z

I think it is enough for this PR, new ideas I will push in another one.

separator log_separator is not working properly for other layouts than layout_simple #95
After many tests I think that the new utils function catch_base_log is the best way to get log header length (nchar). It might return wrong number of chars if log header has different length under different environments.
Another idea which was not implemented is to add width argument to logger -> log_level. Thus control message/output length at the creation level.
I provide informative roxygen2 docs when log_separator is not perfectly supported.
list(...) unpacking list(...) #92 For list(..) I transfer part of the code.
DRY rule for log_* family functions - a new template function log_base
ifelse -> if else - where input is singular. ifelse is recommended for vectors where as for single logical we should use if else

* test * fix new fun for custom appender * docs

daroczig · 2021-12-01T09:57:47Z

R/appenders.R

                    suppressWarnings(R.utils::countLines(file)),
                    error = function(e) 0)
-                n_bytes <- ifelse(file.exists(file), file.info(file)$size, 0)
+                n_bytes <- if (file.exists(file)) file.info(file)$size else 0


what's the advantage of calling if/else (without the curly braces) compared to ifelse?

It is more secure and readable, when working with scalars.
Here a Hadley Wickham opinion who is in favour of such step.
https://twitter.com/hadleywickham/status/928983687605059585

Strongly agree with @Polkas . Note also this edge case (not relevant for file.exists(file), since it can not return NA, but still):

> ifelse(NA, TRUE, FALSE) [1] NA > `if`(NA, TRUE, FALSE) Error in if (NA) TRUE else FALSE : missing value where TRUE/FALSE needed

Note also that for simple conditionals, I tend to use backticked if to keep the functional style.

Hm, got it, thanks for the pointers. Although makes sense from a perspective, but I have an aversion to using if without the curly braces, as have been hurt by how the parser handles it (single VS multiple statements), e.g. with covr false positives or missed else branches on multiline expressions.

Although I've not run into this issue recently, habits die hard :)

And the R manual of if still suggests always keeping the braces .. which makes it ugly for single-line statements.

Anyway, I will keep thinking about it, and looking at the rest of the PR in the meanwhile -- thanks again!!

I understand your doubts, so what about ifelse(isTRUE(cond), ..., ...).

Sorry, let's not wrap file.exists in isTRUE 🤐

I don't think there's any benefit in that extra check, still makes the code more complex and a bit slower.

So far, the backtick + if combo is the most appealing to me .. but still feels like black magic, so probably we need something easier to grasp -- which might be just the ugly multi-line if conditionals?

Anyway, this is more of a language syntax question, and I feel like we just miss a nice R implementation of the ternary operator, which is unlikely that we will resolve in this thread :)

So I might be open to updating the above to avoid if conditionals altogether with something like:

na.omit(c(file.info(file)$size, 0))[1]

But let me rather check the rest of the PR instead at last :)

R/utils.R

daroczig · 2021-12-02T07:57:48Z

R/utils.R

+    res <- capture.output(log_level(level = level, namespace = namespace), type = "message")
+    if (length(res) == 0) {
+        # catch output
+        res <- capture.output(log_level(level = level, namespace = namespace), type = "output")
+    }


do we need to check both stderr and stdout? setting appender_console (which is an alias to appender_stderr) should (in theory) write to the former, and nothing to the latter.

I added the log_appender(appender_console) call as I got double prints in some tests. Preciously for helpers testing where there is used log_appender(appender_stdout). catch_base_log should be neutral for the rest of the session, that is why I am doing stderr redirection (2>).

double prints should not happen, except if namespaces and/or indexes are messed up. I think passing namespace in the other related comment, and systematically everywhere in this function might resolve it?

You are right namespaces were a clue. More than that I had to use fallback_namespace.

#' catch_base_log(INFO, NA_character_) #' logger <- layout_glue_generator(format = '{node}/{pid}/{namespace}/{fn} {time} {level}: {msg}') #' log_layout(logger) #' catch_base_log(INFO, NA_character_) #' fun <- function() catch_base_log(INFO, NA_character_) #' fun() #' catch_base_log(INFO, NA_character_, .topcall = call("funLONG")) #' } catch_base_log <- function( level, namespace, .topcall = sys.call(-1), .topenv = parent.frame() ) { namespace <- fallback_namespace(namespace) orginal_appender <- log_appender(namespace = namespace) log_appender(appender_console, namespace = namespace) # catch error, warning or message res <- capture.output( log_level( level = level, namespace = namespace, .topcall = .topcall, .topenv = .topenv ), type = "message" ) log_appender(orginal_appender, namespace = namespace) res }

getting namespace and if not exists then return global.

getting current namespace appender, to be able to return to it later.

temporally setting appender_console (stderr) inside the namespace and catch (run) capture.output(log_layout(level, namespace = namespace), type = "message"). log_layout will not print to the console as we capture an expected stderr.

Go back to the original appender and apply it to the namespace

return the catched header (from log_layout), we could e.g. count its characters for log_separator

We get a log_layout header and do not influence the environment

Please read on github as I added small edit.

In the one of commits I added testthat tests for the catch_base_log function.

I start to consider .topcall so the function is even more secure when the layout depends on where the function is called. I added .topcall argument to log_separator too. More tests are added for these scenerios.

R/logger.R

daroczig · 2021-12-05T22:21:00Z

sorry for the delay with this - got distracted by #98 and the weekend .. I will pick it up again in a few days

R/helpers.R

Co-authored-by: Gergely Daróczi <daroczig@rapporter.net>

daroczig · 2021-12-30T23:47:57Z

This is great, thank you very much again! Also thanks for the ping in the other PR on the Travis -> GH actions update 👍

I am ready to merge this, but may I ask the below changes before that?

Can you please revert the ifelse updates? I don't see a clear winner at the time being, so let's delay that discussion to a future PR.
Can you please revert the code style changes in the old code? E.g. breaking long roxygen lines. And update the newly added lines to conform with the rest of the codebase: single quotes, 4 spaces.

No problem if not, I can totally do that, just let me know.

Polkas · 2022-01-01T20:26:43Z

I reverted ifelse, " quote and roxygen2 long lines split. Moreover I go through new code and update it to use 4 spaces.

Have a great new year:)

daroczig · 2022-01-01T20:49:04Z

tests/testthat/test-utils.R

+    expect_true(nchar(logger:::catch_base_log(INFO, NA_character_)) == 0)
+    log_layout(layout_original)
+    layout_original <- log_layout(namespace = 'TEMP')
+    logger <- layout_glue_generator(format = '{node}/{pid}/{namespace}/{fn} {time} {level}: {msg}')


I suspect the PR builder checks (tests) are failing due to this .. nod and pid can have variable with, maybe test without those?

see e.g. https://github.com/daroczig/logger/runs/4681051142?check_suite_focus=true

I change the computer and now it works on both of them. You are right node and pid have to be removed from tests as are machine specific. Good finding, you save a lot of my time:) Of course now it looks to be obvious.

ah looks like default time has different format on old Linux machine. I will remove it from layout and we should be at home. Rest of machines are ok.

codecov-commenter · 2022-01-02T19:45:53Z

Codecov Report

Merging #96 (98f0f09) into master (ca896c7) will increase coverage by 1.65%.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##           master      #96      +/-   ##
==========================================
+ Coverage   79.11%   80.76%   +1.65%     
==========================================
  Files          10       10              
  Lines         522      520       -2     
==========================================
+ Hits          413      420       +7     
+ Misses        109      100       -9

Impacted Files	Coverage Δ
R/helpers.R	`100.00% <100.00%> (ø)`
R/logger.R	`98.85% <100.00%> (+7.70%)`	⬆️
R/utils.R	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca896c7...e9c6acf. Read the comment docs.

Polkas · 2022-01-02T21:51:31Z

Luckily I have a pythonanywhere account, with a Linux and the old R version (3.6.3).

Looks like we find some edge case.

should be "TEMP/nchar INFO: " and we get "TEMP/eval INFO: " (.topcall == eval(expr, pf) not nchar(logger:::catch_base_log(...)))

daroczig · 2022-01-02T22:37:48Z

I am pretty sure that is due to testtat messing with the scopes, see e.g. https://github.com/daroczig/logger/blob/master/tests/testthat/test-CRANSKIP-logger-namespaces.R#L18

One workaround, just like above, is making the call happen outside of testthat .. or just remove the fn from the check as well 🤐

Polkas · 2022-01-02T23:36:19Z

Finally I removed one line (one test), as I think we should still test how.topcall influence the call length.
If you prefer other solution, of course I am leaving it to you.)

daroczig

Thanks a lot again for this, and sorry for the back-and-forths 😅 This was a great idea and excellent contribution 🙇

Polkas added 2 commits November 8, 2021 23:31

0 errors and few issues solved

18305f1

catch def

6441d20

* test * fix new fun for custom appender * docs

Polkas marked this pull request as ready for review November 21, 2021 19:53

daroczig reviewed Dec 1, 2021

View reviewed changes

daroczig reviewed Dec 2, 2021

View reviewed changes

R/utils.R Outdated Show resolved Hide resolved

daroczig reviewed Dec 2, 2021

View reviewed changes

R/logger.R Show resolved Hide resolved

daroczig reviewed Dec 2, 2021

View reviewed changes

R/logger.R Outdated Show resolved Hide resolved

daroczig reviewed Dec 2, 2021

View reviewed changes

R/logger.R Show resolved Hide resolved

Polkas added 6 commits December 2, 2021 23:56

update after discussion

6569ee1

testthat catch_base_log

bf48a4a

apply catch_base_log to log_with_separator

67aea44

rm files

1c88f6a

support length in all cases .topcall

2decb0b

Merge branch 'devel' of https://github.com/Polkas/logger into devel

832acb8

daroczig reviewed Dec 29, 2021

View reviewed changes

R/helpers.R Outdated Show resolved Hide resolved

Update R/helpers.R

3704ec4

Co-authored-by: Gergely Daróczi <daroczig@rapporter.net>

Polkas added 5 commits January 1, 2022 20:27

Merge branch 'master' into devel

82496b0

revert ifelse and roxygen2 split of long lines

af1acb3

quote and style

a77c3d9

style

1366a95

style

52a4f13

daroczig reviewed Jan 1, 2022

View reviewed changes

pass tests - rm machine specific layout

d9680f7

Polkas added 2 commits January 2, 2022 20:58

old Linux time format

7d3c07f

old Linux time format 2

98f0f09

remove one test

e9c6acf

daroczig approved these changes Jan 3, 2022

View reviewed changes

daroczig merged commit cc4b608 into daroczig:master Jan 3, 2022

Comments

Conversation

Polkas commented Nov 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Polkas Dec 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Polkas Dec 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Polkas Dec 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Polkas Dec 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daroczig commented Dec 5, 2021

Uh oh!

Uh oh!

daroczig commented Dec 30, 2021

Uh oh!

Polkas commented Jan 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jan 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Polkas commented Jan 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daroczig commented Jan 2, 2022

Uh oh!

Polkas commented Jan 2, 2022

Uh oh!

daroczig left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Polkas commented Nov 8, 2021 •

edited

Loading

Polkas Dec 1, 2021 •

edited

Loading

Polkas Dec 2, 2021 •

edited

Loading

Polkas Dec 3, 2021 •

edited

Loading

Polkas Dec 4, 2021 •

edited

Loading

codecov-commenter commented Jan 2, 2022 •

edited

Loading

Polkas commented Jan 2, 2022 •

edited

Loading