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

#87 add instance for time of day #196

Merged
merged 8 commits into from
Nov 9, 2019
Merged

#87 add instance for time of day #196

merged 8 commits into from
Nov 9, 2019

Conversation

anneloreegger
Copy link
Contributor

@anneloreegger anneloreegger commented Jun 22, 2019

fix #87

@@ -30,6 +34,7 @@ spec = do
context "Unary records" $ do
context "Email (unary record)" $ checkToParamSchema (Proxy :: Proxy Email) emailSchemaJSON
context "UserId (non-record newtype)" $ checkToParamSchema (Proxy :: Proxy UserId) userIdSchemaJSON
context "TimeOfDay" $ checkToParamSchema (Proxy :: Proxy TimeOfDay) (Object (HM.fromList [("format",String "hh:MM:ss"),("type",String "string")]))
Copy link
Member

Choose a reason for hiding this comment

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

Could you put schema's JSON value in SpecCommon, just like colorSchemaJSON and others?

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

@@ -110,6 +115,7 @@ spec = do
context "MyRoseTree' (inlineNonRecursiveSchemas)" $ checkInlinedRecSchema (Proxy :: Proxy MyRoseTree') myRoseTreeSchemaJSON'
describe "Bounded Enum key mapping" $ do
context "ButtonImages" $ checkToSchema (Proxy :: Proxy ButtonImages) buttonImagesSchemaJSON
context "TimeOfDay" $ checkToSchema (Proxy :: Proxy TimeOfDay ) (Object (HM.fromList [("example",String "12:33:15"),("format",String "hh:MM:ss"),("type",String "string")]))
Copy link
Member

Choose a reason for hiding this comment

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

Could you put schema's JSON value in SpecCommon, just like colorSchemaJSON and others?

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

@anneloreegger anneloreegger changed the title 87 add instance for time of day #87 add instance for time of day Aug 29, 2019
@anneloreegger
Copy link
Contributor Author

Are there any news on this issue?

@fisx
Copy link
Collaborator

fisx commented Sep 29, 2019

Could you please fix the merge conflict? I have commit authority here now and will make sure this gets merged once you have.

Thanks!

…_TimeOfDay

# Conflicts:
#	test/Data/Swagger/SchemaSpec.hs
@anneloreegger
Copy link
Contributor Author

Sorry I'm very busy lately. But now I fixed the merge conflict. So it should hopefully be ready to be merged :)

Copy link
Collaborator

@fisx fisx left a comment

Choose a reason for hiding this comment

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

Thanks! Two more comments (sorry, but I hope they are easy).

Unless anybody requests more changes, I will merge this on ~2019-11-08.

@@ -105,6 +108,7 @@ spec = do
context "MyRoseTree' (inlineNonRecursiveSchemas)" $ checkInlinedRecSchema (Proxy :: Proxy MyRoseTree') myRoseTreeSchemaJSON'
describe "Bounded Enum key mapping" $ do
context "ButtonImages" $ checkToSchema (Proxy :: Proxy ButtonImages) buttonImagesSchemaJSON
context "TimeOfDay" $ checkToSchema (Proxy :: Proxy Data.Time.LocalTime.TimeOfDay ) timeOfDaySchemaJSON
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
context "TimeOfDay" $ checkToSchema (Proxy :: Proxy Data.Time.LocalTime.TimeOfDay ) timeOfDaySchemaJSON
context "TimeOfDay" $ checkToSchema (Proxy :: Proxy Data.Time.LocalTime.TimeOfDay) timeOfDaySchemaJSON

-- >>> toParamSchema (Proxy :: Proxy TimeOfDay) ^. format
-- Just "hh:MM:ss"
instance ToParamSchema TimeOfDay where
toParamSchema _ = timeParamSchema "hh:MM:ss"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are hh, ss small, but MM is capitalized? Could you explain this in the haddocks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I am not sure if it makes a difference if MM is capitalized or not. I just took the schema that was used for LocalTime ("yyyy-mm-ddThh:MM:ss") and removed the parts I didn't need.
So basically I left the M capitalized for being consistent, but I don't know if there is another reason.

Copy link
Member

Choose a reason for hiding this comment

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

mm is month. MM is minutes.

@fisx fisx merged commit 1874b98 into GetShopTV:master Nov 9, 2019
@fizruk fizruk mentioned this pull request Nov 23, 2019
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.

Add instances for TimeOfDay
3 participants