tmastny / browse

Navigate and generate links to remote files
Other
29 stars 0 forks source link

`browse()` is missing the "branch" #2

Closed pat-s closed 4 years ago

pat-s commented 4 years ago

when calling browse::browse() in RStudio v1.3.698 on macOS Mojave 10.14.6 I get

https://github.com/mlr/NA/blob/e12475364a8412e94b19b99d329ed98b5d174c8f/tests/testthat/test_classif_dbnDNN.R#L34-L34

tmastny commented 4 years ago

Thanks for the issue!

Something definitely seems wrong:

https://github.com/mlr/NA/...

strikes me as a malformed URL. I think it should be

https://github.com/mlr-org/mlr...

What file and on what branch are you trying to browse to? That will help me reproduce and debug.

When I fork the repo it seems to work: https://github.com/tmastny/mlr/blob/e12475364a8412e94b19b99d329ed98b5d174c8f/tests/testthat/test_classif_dbnDNN.R#L11-L14

One possible issue is that browse (unlike your solution in the usethis PR) doesn't use the branch name to generate the link. It uses the commit hash in the current head of the repo. This is intentional, because my original purpose was to create permalinks to send to coworkers. If I just used the branch name, then the contents of the file could change as the branch changes.

However, I don't think this is the issue, because of the strange NA in the URL.

tmastny commented 4 years ago

Oh and another thought: the commit (and the branch) must be on Github for browse to work.

pat-s commented 4 years ago

One possible issue is that browse (unlike your solution in the usethis PR) doesn't use the branch name to generate the link. It uses the commit hash in the current head of the repo. This is intentional, because my original purpose was to create permalinks to send to coworkers. If I just used the branch name, then the contents of the file could change as the branch changes.

Using the commit hash is definitely more robust. My implementation was just a first shot.

I was just using my RStudio project of the {mlr} repo, nothing special.


The problem is here

library(browse)

repo_url = "git@github.com:pat-s/browse"
owner <- repo_url %>%
  urltools::path() %>%
  stringr::str_split("/")

list(
  #domain = domain,
  owner = owner[[1]][1],
  name = owner[[1]][2]
  #sha = commit$sha
)
#> $owner
#> [1] "browse"
#> 
#> $name
#> [1] NA

Created on 2020-01-07 by the reprex package (v0.3.0)

Maybe you're not accounting for SSH?

tmastny commented 4 years ago

That's it! I forgot about SSH. Thanks for the help debugging! I should have a fix tonight (I'd also welcome PRs!).

pat-s commented 4 years ago

Thanks, works now for me! I really like it. Huge productivity booster for me.

Not sure if {usethis} is ever willing to include all of this functionality but you should def. promote this one better. Maybe posting it at rweekly.org could be a good start :)