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

Allows port to be specified as an environment variable #963

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

shikokuchuo
Copy link
Member

@shikokuchuo shikokuchuo commented Nov 3, 2024

Fixes #956 (tagged 'help wanted').

The error returned there is an Rcpp error, as the httpuv function that's ultimately called is expecting an integer input and can't handle the implicit character -> integer conversion. Environment variables are by default retrieved as strings.

I've added the as.integer() conversion as a catch-all, rather than add a conditional branch as:

i) I find it slightly more readable
ii) slightly more defensive, in case the methods to find a random port above it are updated
iii) getRandomPort() returns a double so there would have been an implicit conversion at the Rcpp level anyway

Includes a regression test.

PR task list:

  • Update NEWS
  • Add tests
  • Update documentation with devtools::document()

@CLAassistant
Copy link

CLAassistant commented Nov 3, 2024

CLA assistant check
All committers have signed the CLA.

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.

Plumber crashes when specifying port via environment variable
2 participants