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

yesodSpecWithSiteGenerator with an argument #1485

Merged
merged 2 commits into from Apr 30, 2018

Conversation

Projects
None yet
2 participants
@NorfairKing
Contributor

NorfairKing commented Feb 6, 2018

Before submitting your PR, check that you've:

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

^ I'll complete the checkboxes if you approve of the change.

Show outdated Hide outdated yesod-test/Yesod/Test.hs Outdated
@NorfairKing

This comment has been minimized.

Show comment
Hide comment
@NorfairKing

NorfairKing Apr 21, 2018

Contributor

Let's first see if we can agree that this addition is useful.

What's the purpose of this function?
Why would I use this function instead of yesodSpecWithSiteGenerator?

The use-case is as follows:

I have a Yesod site that contains the information necessary to call an external web service: Url and port, for example.
I am also the author of the external web service and would like to test how these two interact.
I would like to use testWithApplicationSettings to start the web service around the test.
That means I need something like SpecWith Wai.Application.
The port automatically gets chosen when this application is started, so the Yesod site needs to be made with the IO Site function only after this port is available.
This is why I need an argument to the IO Site function. (It is the yesod equilavent of beforeWith)

I hope the above clears up the need and the purpose of this new function.
I am very open to changing the specifics as long as my use-case can be supported.

We definitely need a Haddock for this. (Also, when written, we need a @SInCE comment, ChangeLog entry, and cabal file version bump.)

Will do that if you agree that this is a useful change.

Contributor

NorfairKing commented Apr 21, 2018

Let's first see if we can agree that this addition is useful.

What's the purpose of this function?
Why would I use this function instead of yesodSpecWithSiteGenerator?

The use-case is as follows:

I have a Yesod site that contains the information necessary to call an external web service: Url and port, for example.
I am also the author of the external web service and would like to test how these two interact.
I would like to use testWithApplicationSettings to start the web service around the test.
That means I need something like SpecWith Wai.Application.
The port automatically gets chosen when this application is started, so the Yesod site needs to be made with the IO Site function only after this port is available.
This is why I need an argument to the IO Site function. (It is the yesod equilavent of beforeWith)

I hope the above clears up the need and the purpose of this new function.
I am very open to changing the specifics as long as my use-case can be supported.

We definitely need a Haddock for this. (Also, when written, we need a @SInCE comment, ChangeLog entry, and cabal file version bump.)

Will do that if you agree that this is a useful change.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Apr 23, 2018

Member

I wasn't objecting to the inclusion of the function, something which takes an argument like this seems fine. But it needs a more descriptive name and Haddocks.

Also, while we're at it: can't yesodSpecWithSiteGenerator be rewritten to use the new function, and avoid some code duplication?

Member

snoyberg commented Apr 23, 2018

I wasn't objecting to the inclusion of the function, something which takes an argument like this seems fine. But it needs a more descriptive name and Haddocks.

Also, while we're at it: can't yesodSpecWithSiteGenerator be rewritten to use the new function, and avoid some code duplication?

@NorfairKing

This comment has been minimized.

Show comment
Hide comment
@NorfairKing

NorfairKing Apr 28, 2018

Contributor

@snoyberg Ready for more review.

Contributor

NorfairKing commented Apr 28, 2018

@snoyberg Ready for more review.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Apr 30, 2018

Member

Thanks!

Member

snoyberg commented Apr 30, 2018

Thanks!

@snoyberg snoyberg merged commit 4cd29ae into yesodweb:master Apr 30, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment