Skip to content

Conversation

cscheid
Copy link
Collaborator

@cscheid cscheid commented Mar 29, 2023

(cc @jennybc)

This PR makes it so that search() doesn't get polluted by the tools:quarto attach() call, which is really only needed on OJS settings.

@cscheid cscheid requested a review from jjallaire March 29, 2023 04:16
Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

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

Small comment on the regex.

Otherwise, regarding the duplication, I think attach() is used to put in search path what they call a database. So when you call it twice with same name, it will add a new one and not assign the what value to it.

You need to use assign() for that.

So you could do

  • .tools_env <- attach(NULL, name = "tools:quarto") to create the environment. This is mentioned in the help page

    One useful ‘trick’ is to use what = NULL (or equivalently a length-zero list) to create a new environment on the search path into which objects can be assigned by assign()

  • Then use assign(..., envir = .tools_env) when you need to add to that one

This is how I would do it.

As I remembered RStudio IDE is also using a tools:rstudio, I checked and they are doing this kind of things, with more complex tricks for some reason (using a function to get the environment as an environment if it already exists, I don't think we need that).
Anyhow, you can see here
https://github.com/rstudio/rstudio/blob/68c31072ae07761aab3747ead8044b729cfe9cb4/src/cpp/r/R/Tools.R#L16-L22

Hope it helps

# we need ojs only if markdown has ojs code cells
# inspect code cells for spaces after line breaks

needs_ojs <- grepl("\n[[:space:]]*```+\\{ojs\\}", markdown)
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW this is the pattern we use in knitr to detect chunks
https://github.com/yihui/knitr/blob/1239f36c5dee8b0e029958a793ae71558902f52c/R/pattern.R#L32

^[\t >]*```+\\s*\\{([a-zA-Z0-9_]+( *[ ,].*)?)\\}\\s*$

A bit more complex regex but I think this is because it covers more case in knitr; like options in chunk header and we match on line. So seems good.

Only two comments on the pattern:

  • I think [[:space:]] covers new line so you don't need \n before.
  • Are we really strict on {ojs} exactly ? Nothing else can be in the { } ? We support a bit more in knitr
    • for example I think a space works currently like ```{ojs } works
    • Could there be any options ?

Just sharing as this could breaks some usage (that we could see as bad syntax, but it works today)

This could mean using \\{ojs( *)?\\} or \\{ojs[^}]*\\}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This could mean using \\{ojs( *)?\\} or \\{ojs[^}]*\\}

Either one of these is better than what I have, you're totally right.

@cscheid
Copy link
Collaborator Author

cscheid commented Mar 29, 2023

So you could do

  • .tools_env <- attach(NULL, name = "tools:quarto") to create the environment. This is mentioned in the help page

    One useful ‘trick’ is to use what = NULL (or equivalently a length-zero list) to create a new environment on the search path into which objects can be assigned by assign()

  • Then use assign(..., envir = .tools_env) when you need to add to that one

This is how I would do it.

Yes, I agree that this is how we should do that. My first goal was to hide them at all because Jenny needed a clean search() result for the book she's writing.

The reason I didn't do it right away is that the two calls to attach happen in separate files, and I don't know the semantics of source() well enough to understand what happens with the variable assignment and the associated scope. But I'd appreciate the help in fixing that!

@cscheid cscheid force-pushed the feature/hide-tools-quarto-attach branch from 2ab6766 to 0d0c35f Compare March 29, 2023 16:35
@dragonstyle dragonstyle added this to the v1.3 milestone Mar 29, 2023
@dragonstyle dragonstyle merged commit 0ef47ca into main Mar 29, 2023
@dragonstyle dragonstyle deleted the feature/hide-tools-quarto-attach branch March 29, 2023 17:08
cderv added a commit that referenced this pull request Sep 8, 2023
…nitr to find during knitting

This avoids duplication of environment, as discussed in #5012 (review)
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.

3 participants