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

Feature/62 optional logging of conditions #67

Merged
merged 3 commits into from
Oct 19, 2021
Merged

Feature/62 optional logging of conditions #67

merged 3 commits into from
Oct 19, 2021

Conversation

vwrobel
Copy link
Contributor

@vwrobel vwrobel commented Aug 31, 2021

I chose to have the default (no logging of conditions) logged.conditions to NULL instead of "" so as to keep consistency with an empty string vector c() . Let me know what you think. Cheers.

@aryoda
Copy link
Owner

aryoda commented Sep 1, 2021

Wow, I didn't expect the PR so soon and even containing unit tests :-) Thanks a lot for your contribution!

Excellent idea, using NULL instead of "", I totally agree with this semantics.

I am on holiday ATM and will look deeper into the code and unit test results this weekend.

A few first things I saw in the code:

  1. It looks like conditions are blocked (suppressed) so that the caller cannot handle the conditions.
    I think the parameter was meant to only block the logging output but let the conditions still bubble up.
    Since (custom) conditions do not create output in R by default it is invisible in the console anyhow:
    rlang::signal("a custom condition", "ConditionClass1").
    I would like to change this behaviour (the parameter should affect the logging output only)
  2. I am thinking of renaming the parameter to "log.condition.classes" or similar for a clearer name
    (once the update is on CRAN we must stick to the name for API compatibility reasons)
  3. I will add some more unit tests regarding the expcected log output and fix some (currently broken) unit tests (reported by CI system)

Again: Thanks a lot for the code contribution, I will come back here this weekend.

BTW: For legal reasons:

  1. Do you accept using the GPL3 license for your code contribution?
  2. May I add your name as contributor to the DESCRIPTION file of the package? Which email address should I use for that?

@vwrobel
Copy link
Contributor Author

vwrobel commented Sep 1, 2021

Hey,
This package is very helpful to me. The debug from dumpfile is a game changer for post mortem of an app in production. I was glad to contribute. Even to a small extent.
Of course I accept the GPL3 license. My e-mail : [email protected].
I agree regarding 2. "log.condition.classes" is better. As for 1. I see I did not handle custom conditions the proper way. I'll see on your code.
Enjoy your holidays!

@aryoda
Copy link
Owner

aryoda commented Sep 6, 2021

This PR is a proposed implementation for issue #62

@aryoda aryoda merged commit 5490629 into aryoda:master Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work in progess is currently in the implementation phase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants