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

Add runEnv: like run but uses $PORT #334

Merged
merged 2 commits into from Feb 8, 2015
Merged

Add runEnv: like run but uses $PORT #334

merged 2 commits into from Feb 8, 2015

Conversation

pbrisbin
Copy link
Member

@pbrisbin pbrisbin commented Feb 6, 2015

When making smaller Yesod sites not using the scaffold, by far the most common
way I run it is to look at $PORT then fall back. I thought maybe the helper
we've been using for this would be generally useful. It would certainly be nice
not to have to define it again in all our projects.

As usual, I'm not tied to the name.

If this is accepted, and when the version bump happens, please also update the
"Since" comment.

@gregwebs
Copy link
Member

gregwebs commented Feb 6, 2015

Would it make more sense to call this something like runPortEnv since it is only checking the port?

@pbrisbin
Copy link
Member Author

pbrisbin commented Feb 6, 2015

Maybe. I don't find that any clearer or more intention-revealing, myself.

I went with runEnv because where run uses the the Port given, runEnv gets it from the environment -- neither mention Port in their name even though both involve one. In other words, I'd be more on board with runPortEnv if run were already named runPort.

@snoyberg
Copy link
Member

snoyberg commented Feb 7, 2015

I'm OK with this as-is. @gregwebs I'll hold off on merging based on your objections.

@gregwebs
Copy link
Member

gregwebs commented Feb 7, 2015

I agree with @pbrisbin's reply. However, I do object to the use of the partial read function. It is fine to throw an exception, but it should mention the input value that was read in from the env variable and runEnv.

@pbrisbin
Copy link
Member Author

pbrisbin commented Feb 7, 2015

That's a good point. I should be able to make that change on Monday.
On Feb 7, 2015 6:38 PM, "Greg Weber" notifications@github.com wrote:

I agree with @pbrisbin https://github.com/pbrisbin's reply. However, I
do object to the use of the partial read function. It is fine to throw an
exception, but it should mention the input value that was read in from the
env variable and runEnv.


Reply to this email directly or view it on GitHub
#334 (comment).

@pbrisbin
Copy link
Member Author

pbrisbin commented Feb 8, 2015

I went with fail as a first swing since it seemed like the simplest thing that would work. If you'd prefer throwing some kind of actual exception type, let me know.

Another option would be to use the fallback on an invalid value (not just a missing value) but that felt too surprising for an end-user -- I'd rather a mis-configuration result in a useful failure than a silent change in behavior.

@gregwebs
Copy link
Member

gregwebs commented Feb 8, 2015

looks great, thanks!

gregwebs added a commit that referenced this pull request Feb 8, 2015
Add runEnv: like run but uses $PORT
@gregwebs gregwebs merged commit 841b2ec into master Feb 8, 2015
@gregwebs gregwebs deleted the pb-run-env branch February 8, 2015 19:24
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.

None yet

3 participants