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.

Sign up for GitHub

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

Jump to bottom

server: added Config machinery #327

Merged
merged 9 commits into from
Jan 21, 2016
Merged

server: added Config machinery #327

merged 9 commits into from
Jan 21, 2016

Conversation

soenkehahn
Copy link
Contributor

Replacement for #321 with just one commit.

@soenkehahn soenkehahn mentioned this pull request Jan 14, 2016
Closed
jkarni
jkarni reviewed Jan 15, 2016
View reviewed changes
servant-server/test/Servant/Server/Internal/ConfigSpec.hs
(NamedConfig subConfig :: NamedConfig "sub" '[Char])
:. EmptyConfig
subConfig = 'b' :. EmptyConfig
it "allows to extract subconfigs" $ do
Copy link
Member

Choose a reason for hiding this comment

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

to extract ==> extracting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@jkarni
Copy link
Member

jkarni commented Jan 15, 2016

I have purely aesthetic comments - this otherwise LGTM!

@jkarni jkarni mentioned this pull request Jan 15, 2016
Open
@alpmestan
Copy link
Contributor

Great work guys. LGTM.

@soenkehahn
Copy link
Contributor Author

There are a few things that still needs to be done:

  • documentation,
  • writing instances for WithNamedConfig for all the other interpretations.

Should I already merge this so that others can start to work on auth combinators based on this? Or should I wait till we get these done?

@soenkehahn soenkehahn mentioned this pull request Jan 16, 2016
Closed
@alpmestan
Copy link
Contributor

Re documentation, I was assuming the tutorial would be augmented with some paragraphs on basic usage of Config along with another bit that explains how to use Config for storing things used by HasServer instances of in-house combinators.

@jkarni
Copy link
Member

jkarni commented Jan 16, 2016

The policy I thought we were aiming for is that master should always be release-quality, which to me suggests both should be done as part of the PR. I also don't think the tutorial obviates the need for haddocks.

@soenkehahn soenkehahn force-pushed the shahn/config2 branch 2 times, most recently from ddeb8b6 to 8ecc3f0 Compare January 16, 2016 16:06
@soenkehahn
Copy link
Contributor Author

@alpmestan: Yeah, the tutorial should include something like that. But that'll be a different repo. (And I don't think we should aim for that being in lockstep with master all the time.)

I just didn't want to block work on the auth combinator. But then I'll say this: 8ecc3f0 is a commit that auth combinators can be based on. I'll refrain from --force pushing another version of that commit. (@jkarni, @aaronlevin)

@aaronlevin
Copy link
Contributor

@soenkehahn thanks, I'll merge that commit and take it from there!

jkarni
jkarni reviewed Jan 18, 2016
View reviewed changes
servant-foreign/src/Servant/Foreign/Internal.hs
@@ -295,6 +295,13 @@ instance HasForeign lang sublayout => HasForeign lang (Vault :> sublayout) where
foreignFor lang Proxy req =
foreignFor lang (Proxy :: Proxy sublayout) req

instance HasForeign lang sublayout =>
HasForeign lang (WithNamedConfig name config sublayout) where
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't there need to be an instance for (WithNamedConfig name config s) :> sublayout too?

@fizruk fizruk mentioned this pull request Jan 20, 2016
Closed
@soenkehahn soenkehahn force-pushed the shahn/config2 branch 2 times, most recently from 51bd43a to 731fd0b Compare January 20, 2016 19:28
soenkehahn
soenkehahn reviewed Jan 20, 2016
View reviewed changes
servant/src/Servant/API/WithNamedConfig.hs
-- parentesizing):
--
-- > type UseNamedConfigAPI2 = WithNamedConfig "myConfig" '[String] (
-- > ReqBody '[JSON] Int) :> Get '[JSON] Int
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jkarni: we don't have an instance for that.

(Could this be doctests?)

@soenkehahn soenkehahn merged commit b9fb80a into master Jan 21, 2016
@soenkehahn soenkehahn deleted the shahn/config2 branch January 21, 2016 17:38
@jkarni jkarni mentioned this pull request Jan 21, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@soenkehahn @jkarni @alpmestan @aaronlevin