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

Run rix_init after rix #231

Merged
merged 23 commits into from
Jul 2, 2024
Merged

Run rix_init after rix #231

merged 23 commits into from
Jul 2, 2024

Conversation

b-rodrigues
Copy link
Contributor

This PR would run rix_init each time rix is executed

@b-rodrigues
Copy link
Contributor Author

b-rodrigues commented Jun 28, 2024

we could also add this: #230 (comment)

EDIT: done

@b-rodrigues
Copy link
Contributor Author

b-rodrigues commented Jun 28, 2024

ready for review

  • rix_init() is now always called when using rix() with create_missing
  • rix() creates the project path folder if it's missing
  • install.packages() is redefined in the .Rprofile if running inside a Nix session to throw an error
  • documentation was adapted

@philipp-baumann
Copy link
Collaborator

philipp-baumann commented Jun 28, 2024

ready for review

* rix_init() is now always called when using rix() with create_missing

* rix() creates the project path folder if it's missing

* install.packages() is redefined in the .Rprofile if running inside a Nix session to throw an error

* documentation was adapted

@b-rodrigues i have to think about it and sleep over it. We really should recommend users to make use of rix::rix_init(), but i see a couple of problems mixing it into rix() because of the hidden effects. Will respond a bit later with concrete examples.

@philipp-baumann
Copy link
Collaborator

philipp-baumann commented Jun 29, 2024

todos like discussed on matrix :-)

  • add message_type option and also forward it to rix_init() called.

@b-rodrigues
Copy link
Contributor Author

todos like discussed on matrix :-)

  • add message_type option and also forward it to rix_init() called.

Implemented, but with a twist: "verbose" is too chatty when rix_init() is called by rix(), so if the user chooses "verbose", it gets "downgraded" to "simple".

@philipp-baumann
Copy link
Collaborator

todos like discussed on matrix :-)

  • add message_type option and also forward it to rix_init() called.

Implemented, but with a twist: "verbose" is too chatty when rix_init() is called by rix(), so if the user chooses "verbose", it gets "downgraded" to "simple".

That doesn't make sense to me. If I choose"verbose", I explicitly want "verbose", and consistent behavior across all functions, here rix_init(). The default is "simple", and one either wants to have all outputs or none as an alternative.

@b-rodrigues
Copy link
Contributor Author

That doesn't make sense to me. If I choose"verbose", I explicitly want "verbose", and consistent behavior across all functions, here rix_init(). The default is "simple", and one either wants to have all outputs or none as an alternative.

The issue is that "verbose" for rix_init() prints many lines that push the lines from rix() out of the console. So my thinking was that if I'm using "verbose" with rix(), I'm more interested to what rix() has to say. I feel like then having rix_init() message_type set to "simple" would be enough for "verbose" for rix().

But if I'm calling rix_init() directly with "verbose", then yeah, I'd like to know everything rix_init() has to tell me.

@philipp-baumann
Copy link
Collaborator

@b-rodrigues currently, we still don't get the expected behavior if we already have an .Rprofile that was generated before outside of nix. How would we know if a user called rix_init() before or not. One might have just a custom .Rprofile but without the necessary code to limit .libPaths() to nix store paths. In one situation it would work while in the second it gets overlooked and we still have interference from the user library.

@b-rodrigues
Copy link
Contributor Author

b-rodrigues commented Jul 1, 2024

@b-rodrigues currently, we still don't get the expected behavior if we already have an .Rprofile that was generated before outside of nix. How would we know if a user called rix_init() before or not. One might have just a custom .Rprofile but without the necessary code to limit .libPaths() to nix store paths. In one situation it would work while in the second it gets overlooked and we still have interference from the user library.

Right, we need to check for existence of an already existing .Rprofile!

  • Check for existence of already existing .Rprofile and warn user.

@philipp-baumann
Copy link
Collaborator

philipp-baumann commented Jul 1, 2024

@b-rodrigues currently, we still don't get the expected behavior if we already have an .Rprofile that was generated before outside of nix. How would we know if a user called rix_init() before or not. One might have just a custom .Rprofile but without the necessary code to limit .libPaths() to nix store paths. In one situation it would work while in the second it gets overlooked and we still have interference from the user library.

Right, we need to check for existence of an already existing .Rprofile!

* [ ]  Check for existence of already existing .Rprofile and warn user.

how do you know its generated?

That doesn't make sense to me. If I choose"verbose", I explicitly want "verbose", and consistent behavior across all functions, here rix_init(). The default is "simple", and one either wants to have all outputs or none as an alternative.

The issue is that "verbose" for rix_init() prints many lines that push the lines from rix() out of the console. So my thinking was that if I'm using "verbose" with rix(), I'm more interested to what rix() has to say. I feel like then having rix_init() message_type set to "simple" would be enough for "verbose" for rix().

But if I'm calling rix_init() directly with "verbose", then yeah, I'd like to know everything rix_init() has to tell me.

rix::rix() does two things and I would want to exactly know what it does. I see two options, either we delete

@b-rodrigues currently, we still don't get the expected behavior if we already have an .Rprofile that was generated before outside of nix. How would we know if a user called rix_init() before or not. One might have just a custom .Rprofile but without the necessary code to limit .libPaths() to nix store paths. In one situation it would work while in the second it gets overlooked and we still have interference from the user library.

Right, we need to check for existence of an already existing .Rprofile!

* [ ]  Check for existence of already existing .Rprofile and warn user.

Why don't we add a rprofile_action argument to rix::rix()? We default to create_missing, but also signal other options. How do we know whether rix::rix_init() .Rprofile is taking action?

@b-rodrigues
Copy link
Contributor Author

@b-rodrigues currently, we still don't get the expected behavior if we already have an .Rprofile that was generated before outside of nix. How would we know if a user called rix_init() before or not. One might have just a custom .Rprofile but without the necessary code to limit .libPaths() to nix store paths. In one situation it would work while in the second it gets overlooked and we still have interference from the user library.

Right, we need to check for existence of an already existing .Rprofile!

* [ ]  Check for existence of already existing .Rprofile and warn user.

how do you know its generated?

I check for the sentence "File generated by rix_init" and don't show the warning if it is found.

Why don't we add a rprofile_action argument to rix::rix()? We default to create_missing, but also signal other options. How do we know whether rix::rix_init() .Rprofile is taking action?

we could, but I think that simply providing a default that works largely 95% of the time with a warning in the rare cases it doesn't could be enough?

@philipp-baumann
Copy link
Collaborator

  • for message_type = "quiet", it should not warn and message:
> rix(
+     r_ver = "latest",
+     r_pkgs = c("dplyr", "ggplot2"),
+     system_pkgs = NULL,
+     git_pkgs = NULL,
+     local_pkgs = NULL,
+     ide = "code",
+     project_path = "_test",
+     overwrite = TRUE,
+     print = FALSE,
+     message_type = "quiet",
+     shell_hook = NULL
+ )
Warning message:
In rix(r_ver = "latest", r_pkgs = c("dplyr", "ggplot2"), system_pkgs = NULL,  :
  

### .Rprofile file already exists. You may want to call rix_init(profile_action = 'append') manually to ensure correct functioning of your Nix environment. ###

@philipp-baumann
Copy link
Collaborator

philipp-baumann commented Jul 1, 2024

  • profile_action -> rprofile_action. We could include something like "Please see ?rix_init for an explanation and further options" in the message. Would make sense to include project_path as first arg.
  • suggestion: use warning() with .call = FALSE to declutter.

@b-rodrigues
Copy link
Contributor Author

  • for message_type = "quiet", it should not warn and message:
> rix(
+     r_ver = "latest",
+     r_pkgs = c("dplyr", "ggplot2"),
+     system_pkgs = NULL,
+     git_pkgs = NULL,
+     local_pkgs = NULL,
+     ide = "code",
+     project_path = "_test",
+     overwrite = TRUE,
+     print = FALSE,
+     message_type = "quiet",
+     shell_hook = NULL
+ )
Warning message:
In rix(r_ver = "latest", r_pkgs = c("dplyr", "ggplot2"), system_pkgs = NULL,  :
  

### .Rprofile file already exists. You may want to call rix_init(profile_action = 'append') manually to ensure correct functioning of your Nix environment. ###

should we keep this warning, even if quiet ? That message only appears if .Rprofile already exists and wasn't created by rix_init, so it would appear very rarely and I'd say in these cases the users would like to be informed. What do you think ?

@philipp-baumann
Copy link
Collaborator

philipp-baumann commented Jul 1, 2024

  • for message_type = "quiet", it should not warn and message:
> rix(
+     r_ver = "latest",
+     r_pkgs = c("dplyr", "ggplot2"),
+     system_pkgs = NULL,
+     git_pkgs = NULL,
+     local_pkgs = NULL,
+     ide = "code",
+     project_path = "_test",
+     overwrite = TRUE,
+     print = FALSE,
+     message_type = "quiet",
+     shell_hook = NULL
+ )
Warning message:
In rix(r_ver = "latest", r_pkgs = c("dplyr", "ggplot2"), system_pkgs = NULL,  :
  

### .Rprofile file already exists. You may want to call rix_init(profile_action = 'append') manually to ensure correct functioning of your Nix environment. ###

should we keep this warning, even if quiet ? That message only appears if .Rprofile already exists and wasn't created by rix_init, so it would appear very rarely and I'd say in these cases the users would like to be informed. What do you think ?

yeah, you have a good point. Let's keep it like this. Technically it's a warning and it is good to inform at least, while message_type infers that it is messaging with message() or cat() (its fair to use cat() like we do in a lot of other places in {rix}. I find message() a bit too aggressive in RStudio because it is also in red and assuming many people use RStudio cat is much nicer for many messages.

Something that just came to my mind: If there is no problem, I would expect that rix::rix() gives me a short message that the nix expression was generated and written todefault.nix at project_path = <project_path>, given message_type = "simple" (changed the default to "simple" in c7e124c).

It would nice to have something like this implemented before we merge; currently, there is none:

> devtools::load_all(".")
ℹ Loading rix
> rix(
+     r_ver = "latest",
+     r_pkgs = c("dplyr", "ggplot2"),
+     system_pkgs = NULL,
+     git_pkgs = NULL,
+     local_pkgs = NULL,
+     ide = "code",
+     project_path = "_test",
+     overwrite = TRUE,
+     print = FALSE,
+     message_type = "simple",
+     shell_hook = NULL
+ )

"simple", message_type))

if(message_type != "quiet"){
message("\n\n### Successfully generated `default.nix` and `.Rprofile` ###\n\n")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding your comment bout a successful run, wouldn't this be it ?

@philipp-baumann
Copy link
Collaborator

philipp-baumann commented Jul 1, 2024

proposition, after 31399a9 we have

> rix(
+     r_ver = "latest",
+     r_pkgs = NULL,
+     system_pkgs = NULL,
+     git_pkgs = NULL,
+     local_pkgs = NULL,
+     tex_pkgs = NULL,
+     ide = c("other", "code", "radian", "rstudio", "rserver"),
+     project_path = ".",
+     overwrite = TRUE,
+     print = FALSE,
+     message_type = "simple",
+     shell_hook = NULL
+ )


### Successfully generated `default.nix`. Keeping `.Rprofile` generated by `rix::rix_init()`###

@b-rodrigues b-rodrigues merged commit e23c9c4 into master Jul 2, 2024
10 of 14 checks passed
@b-rodrigues b-rodrigues deleted the rix_init_after_rix branch July 2, 2024 06:27
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.

2 participants