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

Allow ref/pull format in install_dev #279

Merged
merged 6 commits into from Jun 6, 2019
Merged

Conversation

mkearney
Copy link
Contributor

@mkearney mkearney commented Jan 19, 2019

As requested in #206. Checks and then stores ref/pull format to include in install_repo call.

@gaborcsardi
Copy link
Contributor

gaborcsardi commented Jan 19, 2019

Thanks! I wonder if we could do this with a single regex. E.g. sg like

re_match(c("foo", "bar", "foobar@ref"), "^(?<pkg>[^@#]+)(?<ref>[@#].*)?$")

will split up the input, and then you just add back the ref, like you already do. I would put this in a dev_split_ref() function, so that we can test it.

@mkearney
Copy link
Contributor Author

mkearney commented Jan 19, 2019

Sounds good! I'm on it!

R/install-dev.R Outdated
}

url <- build_url(cran_url, "web", "packages", package, "DESCRIPTION")
url <- build_url(cran_url, "web", "packages", dev_split_ref(package)[["pkg"]], "DESCRIPTION")
Copy link
Contributor Author

@mkearney mkearney Jan 19, 2019

Choose a reason for hiding this comment

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

I don't know if it's the best way to access "pkg" and "ref" values, but I used double brackets and column names.

Copy link
Contributor

@gaborcsardi gaborcsardi Jan 20, 2019

Choose a reason for hiding this comment

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

Yes, that's good, but can we call dev_split_ref only once? Not a big a deal performance-wise, more of a principle. Store the result here, and use it later below.

expect_equal(dev_split_ref("with@v1.0.0.999")[["pkg"]], "with")
expect_equal(dev_split_ref("with#279")[["ref"]], "#279")
expect_equal(dev_split_ref("with#1")[["pkg"]], "with")
})
Copy link
Contributor Author

@mkearney mkearney Jan 19, 2019

Choose a reason for hiding this comment

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

These tests all assume input format is (pkg[(@ref|#pull)]?)

Copy link
Contributor

@gaborcsardi gaborcsardi Jan 20, 2019

Choose a reason for hiding this comment

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

Yes, these look great!

@jimhester jimhester added the tidy-dev-day 🤓 label Jan 20, 2019
Copy link
Contributor

@gaborcsardi gaborcsardi left a comment

Thanks, this looks great! Just one more tiny change, please. Thanks again!

R/install-dev.R Outdated
}

url <- build_url(cran_url, "web", "packages", package, "DESCRIPTION")
url <- build_url(cran_url, "web", "packages", dev_split_ref(package)[["pkg"]], "DESCRIPTION")
Copy link
Contributor

@gaborcsardi gaborcsardi Jan 20, 2019

Choose a reason for hiding this comment

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

Yes, that's good, but can we call dev_split_ref only once? Not a big a deal performance-wise, more of a principle. Store the result here, and use it later below.

expect_equal(dev_split_ref("with@v1.0.0.999")[["pkg"]], "with")
expect_equal(dev_split_ref("with#279")[["ref"]], "#279")
expect_equal(dev_split_ref("with#1")[["pkg"]], "with")
})
Copy link
Contributor

@gaborcsardi gaborcsardi Jan 20, 2019

Choose a reason for hiding this comment

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

Yes, these look great!

jimhester added 4 commits Apr 9, 2019
Seriously github, no newline at the end of the file when using your online editor???
@jimhester
Copy link
Member

jimhester commented Jun 6, 2019

Great work, this package is now better thanks to you!

Thanks!! You are da bomb!

@jimhester jimhester merged commit c5e7e3f into r-lib:master Jun 6, 2019
0 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tidy-dev-day 🤓
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants