Skip to content

Submodules #138

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

Merged
merged 3 commits into from
Aug 20, 2018
Merged

Submodules #138

merged 3 commits into from
Aug 20, 2018

Conversation

jimhester
Copy link
Member

This still needs some tests, but it works for simple cases, e.g. install_github("https://github.com/jonkeane/mocapGrip")

@codecov-io
Copy link

codecov-io commented Aug 13, 2018

Codecov Report

Merging #138 into master will decrease coverage by 10.26%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #138       +/-   ##
=========================================
- Coverage   93.26%    83%   -10.27%     
=========================================
  Files          27     28        +1     
  Lines        1514   1600       +86     
=========================================
- Hits         1412   1328       -84     
- Misses        102    272      +170
Impacted Files Coverage Δ
R/install-remote.R 74.19% <100%> (-19.31%) ⬇️
R/install-github.R 64.82% <100%> (-33.15%) ⬇️
R/submodule.R 100% <100%> (ø)
R/install-git.R 20.73% <0%> (-71.96%) ⬇️
R/github.R 31.03% <0%> (-58.63%) ⬇️
R/deps.R 81.63% <0%> (-11.23%) ⬇️
R/utils.R 85.81% <0%> (-0.8%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c7b63a...f296b2d. Read the comment docs.

@gaborcsardi
Copy link
Member

@jimhester Is this ready?

@jimhester
Copy link
Member Author

@gaborcsardi I was adding some tests, it is ready now if you could review it!

@jimhester
Copy link
Member Author

Oh I guess the only other thing this could maybe have is a git2r version, right now it relies on having a command line git only. I can add it if you think we need it.

@gaborcsardi
Copy link
Member

I think it is fine with system git.

@jimhester jimhester requested a review from gaborcsardi August 20, 2018 11:58
Copy link
Member

@gaborcsardi gaborcsardi left a comment

Choose a reason for hiding this comment

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

Looks great!

R/submodule.R Outdated
# escaping them as \" and \\
double_quoted_string_with_escapes <- '(?:\\\\.|[^"])*'

sections <- regexpr(
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should just include a collated rematch2 package, that would simplify things.

})

test_that("Can install a repo with a submodule", {
dir <- tempfile()
Copy link
Member

Choose a reason for hiding this comment

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

Need to skip if no system git. (If we are skipping the other tests that rely on system git.)

@jimhester
Copy link
Member Author

Ok I added the re_match() from rematch2 (it is self contained in 1 file, so is easy to add), also skipped the test, thank you for the reminder. Let me know if there is anything else you think we should change.

@gaborcsardi
Copy link
Member

gaborcsardi commented Aug 20, 2018

I think it is good now.

I think if we want to be completely correct we would need to use the GIT_DIR (etc.) env vars. So maybe parsing the output of git submodule status etc. is a better approach? But we'll probably never run into this.

I don't think we need to change it now, just sg. to think about next time we write sg for git.

@jimhester
Copy link
Member Author

jimhester commented Aug 20, 2018

The issue with that is the zipball we get from GitHub is not actually a git repository, e.g. it doesn't have a .git directory. That is why I can't just run git submodule init, git submodule update to retrieve the submodules and have to parse them by hand (believe me I tried this first :) ). So more complex things like GIT_DIR and defining submodules in a global config etc. actually don't matter for this case.

@gaborcsardi
Copy link
Member

Got it. Yeah, these would only matter for local installs. All good then!

@jimhester jimhester merged commit 0927172 into r-lib:master Aug 20, 2018
@jimhester
Copy link
Member Author

Thanks for the review!

jimhester added a commit to jimhester/remotes that referenced this pull request Aug 21, 2018
jimhester added a commit to jimhester/remotes that referenced this pull request Aug 24, 2018
jimhester added a commit to jimhester/remotes that referenced this pull request Aug 24, 2018
jimhester added a commit that referenced this pull request Aug 24, 2018
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