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

Fix r hooks when hook repo is a package #1831

Merged
merged 1 commit into from Mar 10, 2021

Conversation

lorenzwalthert
Copy link
Contributor

@lorenzwalthert lorenzwalthert commented Mar 8, 2021

In #1799, we assume env_dir instead of prefix_dir as the package source and install it, which creates a package with the right name (because of DESCRIPTION), but obviously does not expose any API and is hence useless. Hooks that rely on the installed package hence fail. Also removing an (apparently) randomly introduced \ that does not cause any harm.

shutil.copy(prefix.path('renv.lock'), env_dir)
cmd_output_b(
'Rscript', '--vanilla', '-e',
"""\
"prefix_dir <- '" + prefix.prefix_dir + "'" +
Copy link
Member

Choose a reason for hiding this comment

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

this should use an f-string to do formatting, also triple-quoted literals can contain quote characters

the backslash was intentional, actually -- it makes the line numbers of the string match up with the string (and avoids a blank line at the top of your string)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, makes all sense. Was too lazy to check if python < 3.6 is required to make f strings work... should be fixed now. Also, because the prefix_dir is passed into string expression (Rscript -e {expr}), another escaping is required for Windows, as it has backward slashes in the path. Maybe there is a better way to do this but I could not find one... Hope it does not create other unexpected problems.

Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

otherwise looks good -- just a small thing

Comment on lines 95 to 97
prefix_dir <- "{
prefix.prefix_dir.encode('unicode_escape').decode()
}"
Copy link
Member

Choose a reason for hiding this comment

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

I think you can use {prefix.prefix_dir!r} to do this as well -- that'll give you the quotes and then you won't have to do an encoding dance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice. I really start to like Python string manipulations 😀. All checks pass now.

Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

This was referenced Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants