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

initial work on with_trace() #204

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MichaelChirico
Copy link
Contributor

Closes #200

Still some work to be done, but filing now for early feedback, e.g.

  1. API design -- I was a bit unsure the best way to handle the signature. In particular, notice from the test that we always have to specify the trace() arguments by name, otherwise use (what, code, args), which doesn't exactly match what we're used to when using trace() directly. Open to suggestions here
  2. Approach for testing. IIUC trace() basically defaults to using .GlobalEnv to look up objects to be traced, which doesn't play well with the normal approach to writing testthat tests -- as seen here, we have to write to .GlobalEnv to test the default behavior. Feels odd, but also didn't feel right to always supply the unit test environment as an argument. Again, open to suggestions -- probably best is to just add several tests and comments about why they're needed. Or we can try and diverge from the base::trace() behavior and always seek to land in the calling environment.
  3. Other repo-specific nitty-gritty like, file naming, style conventions, etc. e.g. I landed this in R/debug.R with the thought that eventually we could put with_debug() here (or other temp state edits for debugging).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

with_trace()?
1 participant