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

Added the option to import from Github #8858

Merged
merged 18 commits into from
Sep 20, 2024

Conversation

Vitalis95
Copy link
Contributor

Fixes #8823
@lilyclements @rdstern @N-thony , Added the option (R package -button) to install a package directly from GitHub

@rdstern
Copy link
Collaborator

rdstern commented Mar 5, 2024

@lilyclements could you check that this is ok. I notice that the github option does not have the check facility. @Vitalis95 Isn't that still needed, when adding from github?

Copy link
Contributor

@lilyclements lilyclements left a comment

Choose a reason for hiding this comment

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

@Vitalis95 - this is great! There's one comment given in the R code, then two other bits below:

  1. We discussed last week about having a check, but I did a little bit of coding today and was able to sort it out. Can you add the following function into the "stand_alone_functions.R" file:
# Function to check if a repo exists and is in R 
check_github_repo <- function(owner, repo) {
  tryCatch({
    response <- gh::gh("/repos/:owner/:repo", owner = owner, repo = repo, verb = "GET", silent = TRUE)
    if (response$language == "R") {
      return(1)  # Repository exists and is in the R language
    } else {
      return(2)  # Repository exists, but isn't in the R language
    }
  }, error = function(e) {
    return(3)  # Error occurred, repository doesn't exist
  })
}

Then we want to add for the messages something like this:

         Case "1"
                ucrInputMessage.SetText("Package exists in the repo and is ready for installation")
                ucrInputMessage.txtInput.BackColor = Color.LightGreen
            Case "2"
                ucrInputMessage.SetText("Package exists in the repo but is not in the R language")
                ucrInputMessage.txtInput.BackColor = Color.LightGreen
          Case "3"
                ucrInputMessage.SetText("Not a package in the given repo. Perhaps spelled wrongly?")
                ucrInputMessage.txtInput.BackColor = Color.LightCoral
  1. Can you add as a new argument in the function upgrade = "never". This can be hardcoded in.

Otherwise looks good!

Comment on lines 178 to 180
clsPasteFunction.AddParameter("paste", Chr(34) & ucrInputRepositoryName.GetText & "/" & ucrInputTextBoxRPackage.GetText & Chr(34), bIncludeArgumentName:=False)

clsRepositoryFunction.AddParameter("package", clsRFunctionParameter:=clsPasteFunction, iPosition:=0, bIncludeArgumentName:=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need your paste function here since you've already combined it with your code! So you can just change this to

clsRepositoryFunction.AddParameter("paste", Chr(34) & ucrInputRepositoryName.GetText & "/" & ucrInputTextBoxRPackage.GetText & Chr(34), bIncludeArgumentName:=False)

And then you can remove the bit on line 180, and remove clsPasteFunction entirely :)

@Vitalis95
Copy link
Contributor Author

@lilyclements , have a look at it

Copy link
Contributor

@lilyclements lilyclements left a comment

Choose a reason for hiding this comment

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

@Vitalis95 this is nice that you've removed the paste0 function, and worked on that.

There's a few changes with the check needed for the GitHub option.

My previous comment gave a function as well as suggestions to the code to amend for this check. Does it make sense to you? It's ok if not, I do need to work on my explanations!

Then another point is that I noticed the tab order is out. Can you sort this as well?

@Vitalis95
Copy link
Contributor Author

@lilyclements , I've fixed the tab order , I'm researching how we can further enhance the suggestions you provided

@Vitalis95
Copy link
Contributor Author

@lilyclements , have a look at this

@lilyclements
Copy link
Contributor

This is good - I have tidied up the design a little bit, so before you make further changes then make sure that you pull the most recent changes. If you don't agree with the changes, we can revert the commit (it's very minor bits of aligning etc).

I can't see the addition of the check_github_repo function that I mentioned above though? Is this added in another PR?

@Vitalis95
Copy link
Contributor Author

@lilyclements , I have added check_github_repo function

Copy link
Contributor

@lilyclements lilyclements left a comment

Choose a reason for hiding this comment

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

@Vitalis95 this is good. Just one small thing - is it possible for the OK to be disabled if the check fails (i.e., is in red). This is done elsewhere - like when checking unique in the dlgKeys dialog, if you want to look at the code?

@Vitalis95
Copy link
Contributor Author

@lilyclements , have a look at it

@rdstern
Copy link
Collaborator

rdstern commented Jun 14, 2024

@lilyclements and @Vitalis95 this looks like a nice enhancement. I had a quick look, partly to check that the Help button works, which it does.
I couldn't then test, because I don't have any examples to try. I should check the issue in more detail first perhaps. Is there at least one that you have in mind, that we could include in the Help information?

I wonder also if the repository name field should be wider than the package name? It is a URL and likely to be quite long? Are there any examples that we know we may use? If so, then could this be a drop down, where you can paste a new url, but there are (say) a couple of obvious examples?

@lilyclements
Copy link
Contributor

lilyclements commented Jun 14, 2024

@rdstern some examples from various R packages we've developed at IDEMS:

Repo: IDEMSInternational
Package: rpicsa, epicsawrap, cdms.products, carbonr, rapidpror, openappr, networkGraphsR, instatExtras

@rdstern
Copy link
Collaborator

rdstern commented Jun 14, 2024

Great. How about that repo and sone, or all, those examples in a drop down. The default is still blank, (so the user types or pastes the contents) but this would give at least one example where no typing is needed? And we could use that in the help on how to do this.

@Vitalis95
Copy link
Contributor Author

Great. How about that repo and sone, or all, those examples in a drop down. The default is still blank, (so the user types or pastes the contents) but this would give at least one example where no typing is needed? And we could use that in the help on how to do this.

@lilyclements , what do you think about this?

@lilyclements
Copy link
Contributor

@Vitalis95 I think this is a good idea, but, perhaps I'm a little late with my response. If I'm too late then ignore me!

@Vitalis95
Copy link
Contributor Author

@lilyclements @rdstern , I have added IDEMSInternational as repo and a drop down with above packages, the default is blank

@N-thony
Copy link
Collaborator

N-thony commented Sep 16, 2024

@lilyclements @rdstern , I have added IDEMSInternational as repo and a drop down with above packages, the default is blank

@lilyclements @rdstern can you re-test this?

N-thony
N-thony previously approved these changes Sep 17, 2024
@N-thony
Copy link
Collaborator

N-thony commented Sep 17, 2024

@rdstern over to you

@rdstern
Copy link
Collaborator

rdstern commented Sep 17, 2024

@Vitalis95 and @lilyclements I am not quite sure what is expected here?

a) I first tried with each of the packages in the drop-down menu:

image

This goes green - they all go green. (So I assume none as currently installed, or are not the most current version?)

So I installed the first package. I assume it installed.

Then I tried again and it still was green. Shouldn't it now say the package is already installed?

Some of the options it says are not R packages - but they are still green. What does that mean? I take green to mean I should install it. Can I install a not R package?

b) Then I tried with a climatic package - CDT that is also only in Github? I could not get it so I could install? I first assumed I could find the whole directory and copy it over. Then I got rid of the start as shown below. Am I supposed to be able to install this sort of package or just our own?

image

(Note the instructions are i9n the CDT github directory and they work in the script window. So is this dialog only meant for the IDEMSInternational packages?)

c) Finally I returned to your packages in the drop-down menu. Now I don't know the location any more. I can get it back, by pressing Reset and then going back to the Github. So not a real problem. But couldn't it also give that directory when I choose from the dropdown?

@Vitalis95 I have updated the R function here. When you next work on this, make sure you pull changes to your Visual Studio
@lilyclements lilyclements dismissed stale reviews from N-thony and themself via 7ff4616 September 18, 2024 14:22
@lilyclements
Copy link
Contributor

@rdstern thanks for this review. There are certainly changes in the R code that you have highlighted here. I have worked on these, so let me know what you think:

Installing New Packages
To import "rijaf-iri/CDT", then you would put "rijaf-iri" in the Repo Name, and "CDT" in the package name.

image

This is really great to see how it can be interpreted - perhaps we should have two options: One where you import from a URL, and one like how we have it now?

  • Import from URL - if this is an option, then we just have one input box where the user writes in the URL.
  • Import from ?? - if this selected, then we have it as it is at the moment.

It is very straightforward to do this in R, so I have added it as an option in the R code for now. We don't have to put this in the dialog if you disagree - good to get opinions though @rdstern or @Vitalis95 - what do you think?

Adding more options when importing from GitHub - I've done this on the checks now
Our check_github_repo() function now returns 0-6 depending on the reason (I've done it numerically as I think this is easier for Vitalis when coding it his end. If not then I can easily amend it!

Return Text/Reason to put in Dialog Colour in Dialog? Explanation
0 Package is up to date Orange? OK does not need to be enabled here - this occurs if the package is installed and up to date, so there is no need to reinstall.
1 Package exists in the repo and is ready for installation Green OK can be enabled! We can install the R package as we are updating it
2 Unable to retrieve from GitHub. Check internet connection? Red OK cannot be enabled due to issue with server.
3 Package exists in the repo and is ready for installation Green OK can be enabled! We can install the R package as we are updating it.
4 Package exists in the repo and is ready for installation Green OK can be enabled! We can install the R package as we are installing it for the first time.
5 Package exists but is not in the R language Red Do not enable OK
6 Not a package in the given repo. Check spelling? Red Do not enable OK

@rdstern @Vitalis95 what are your thoughts on this? Hopefully makes it a bit easier for the user if they encounter issues when installing.

# Examples working my end which you may wish to use (Note: These may give different results your end, especially for the first two!)

check_github_repo(url = "https://github.com/rijaf-iri/CDT")  # This gives a 0 for me since I have installed it and it's up to date. If you've installed this one and it's up to date you should get a 0. 
check_github_repo(url = "https://github.com/IDEMSInternational/openappr")  # This gives a 1 for me since I have installed it but not the latest version.
check_github_repo(url = "https://github.com/IDEMSInternational/openappr")  # If you switch off your internet, you can get a 2.
check_github_repo(url = "https://github.com/tidyverse/dplyr") # 3 Package is installed but not from GitHub - because we've installed this one from CRAN
check_github_repo(url = "https://github.com/simonpcouch/anyflights") # Assuming you haven't installed this (I picked a random one!), you should get a 4 because the repo exists and is in the R language
check_github_repo(url = "https://github.com/mvuorre/exampleRPackage") # Assuming you haven't installed this (I picked a random one!), you should get a 4 because the repo exists but is not in the R language
check_github_repo(url = "https://github.com/rijaf-iri/CDT2")  # You should get a 6 because I made this one up.

@rdstern
Copy link
Collaborator

rdstern commented Sep 18, 2024

@lilyclements that's great:
a) Tried CDT and it installed fine. It also confirmed how many packages we have that are out-of date!
b) But when I tried it again it was still green and with the same message. With CRAN packages it would now say it was installed?

c) However with (at least some of) the IDEMS packages the Check doesn't work now. It doesn't give anything now for Rpicsa or epicsawrap::

image

d) With CDMS Products (and carbonr and openappr) it gives the wrong message - I know it isn't a CRAN package!

image

e) With carbonr and rapidpror is gives the following incorrect message!

image

@rdstern
Copy link
Collaborator

rdstern commented Sep 18, 2024

I am musing now. Suppose we rewrite R-Instat to use the 3 main R packages that you have just documented. (You bravely said that might be a boring but feasible job for one of our programmers???!)
Then if occasionally, a small update, was to one of these packages only, then that could be updated this way and would not need everything to be updated to a whole new version?

@lilyclements
Copy link
Contributor

@rdstern glad that (a) is now working.
To (b)-(e) apologies I wasn't clear enough - I've shared the R code for this, but the VB code on the dialog needs to be updated still.
(I changed some of the numbers in this new updated function since we've added in more options. So currently your warnings are misspecified until we update the dialog VB code! I forgot I did this, I'm sorry for the confusion there)

@lilyclements
Copy link
Contributor

lilyclements commented Sep 18, 2024

@rdstern @Vitalis95 I just, I hope, updated the VB code to reflect the new options of 0-6 in this dialog. If you try again, it should hopefully work now!

I haven't done any other bits (like, adding a URL option, if we wanted that)

@lilyclements
Copy link
Contributor

I am musing now. Suppose we rewrite R-Instat to use the 3 main R packages that you have just documented. (You bravely said that might be a boring but feasible job for one of our programmers???!)
Then if occasionally, a small update, was to one of these packages only, then that could be updated this way and would not need everything to be updated to a whole new version?

@rdstern good point! This could be a great "easy" way to get this done

@rdstern
Copy link
Collaborator

rdstern commented Sep 18, 2024

@lilyclements that's great. All the options in your pull down now work and give useful messages. This is all very nice.

I don't think we need the option of the url. The insalling of packages, like CDT works fine.

The only item remaining is to check on packages like CDT and to give the installed message when it is there.

Then let's merge.

@rdstern
Copy link
Collaborator

rdstern commented Sep 19, 2024

@lilyclements I now noticed that as well as importing a package it now seems to check and list all the other packages in R-Instat that are out-of date. That's new and I wonder if it will worry users unnecessarily?

We usually update the packages with successive releases. So gradually they will become not the most recent. But that is not usually a problem for the user?

@Vitalis95
Copy link
Contributor Author

@rdstern , have a look at it.

@rdstern
Copy link
Collaborator

rdstern commented Sep 19, 2024

@Vitalis95 I don't see what's different?

I had 2 minor problems before: and they are still here
a) After installing CDT (example of a github package, when I try again it is still green. I would like it to detect it is installed and tell me.
b) It gives me a (worrying) list of all the installed packages that are out of date. I don't really want that and used not to get it when installing CRAN packages.
image

ucrInputMessage.SetText("Not a package in the given repo. Check spelling?")
ucrInputMessage.txtInput.BackColor = Color.LightCoral
ucrInputMessage.SetText("Error occurred, repository doesn't exist")
ucrInputMessage.txtInput.BackColor = Color.Red
Copy link
Contributor

Choose a reason for hiding this comment

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

@Vitalis95 is there a reason you changed the colours to be Red/Green instead of LightCoral, etc? Just out of interest

@lilyclements
Copy link
Contributor

@rdstern to (a), this is very interesting since I am not having this problem. There is some coding going on with checking your version vs the version on GitHub, but perhaps the check we need to do is slightly different between our computers. I think this is something to investigate "in person" - we can set as an issue for now?

image

To (b), this is something that comes up in R from the code. We can remove this by adding upgrade = "never" as a parameter, e.g.

devtools::install_github("IDEMSInternational/openappr", upgrade = "never")

I've added this in @Vitalis95. If you make any other changes, make sure you pull this branch first (sorry for interfering -- just know you have a lot to do and it's a one-line bit I can actually do!).
@rdstern point (b) is hopefully fixed now?

rdstern
rdstern previously approved these changes Sep 19, 2024
Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

@lilyclements it now doesn't show the out-of-date packages.
It still shows CDT to be installed always, but you say that might be a machine issue.
Anyway I am accepting, because Chris and @N-thony have now changed the "system", so all the packages on installation continue to be in the original directory, while all we install with this dialog, go into a second directory - and R-Instat looks there first!

I am keen to use this new dialog, in the new version to test all this. I also often have problems updateing some CRAN packages, because R-Instat loads them at the start. We perhaps need a list of those so we don't try to update, them, or learn how that is done. @N-thony perhaps you can supply this list, so we can include it in the help?

@N-thony over to you to check and merge. Really good to have this in the forthcoming release.

lilyclements

This comment was marked as duplicate.

Copy link
Contributor

@lilyclements lilyclements left a comment

Choose a reason for hiding this comment

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

@Vitalis95 thanks for changing the colours. Very minor, but very appreciated! I'll approve since @rdstern was happy with it before this minor change and this commit made his approval stale.

@N-thony over to you to check and merge

PS: Might help if I actually click "Approve" this time (hence the removed comment above)

@N-thony N-thony merged commit 628e099 into IDEMSInternational:master Sep 20, 2024
2 checks passed
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.

Add the option to import from Github
4 participants