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

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

@gaborcsardi
Copy link
Member

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

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

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
Member

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

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
Member

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 🤓 Tidyverse Developer Day rstd.io/tidy-dev-day label Jan 20, 2019
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.

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
Member

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
Member

Choose a reason for hiding this comment

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

Yes, these look great!

Seriously github, no newline at the end of the file when using your online editor???
@jimhester
Copy link
Member

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tidy-dev-day 🤓 Tidyverse Developer Day rstd.io/tidy-dev-day
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants