Skip to content

Switch to classy field lenses#41

Merged
fizruk merged 6 commits into
masterfrom
make-fields-#39
Jan 22, 2016
Merged

Switch to classy field lenses#41
fizruk merged 6 commits into
masterfrom
make-fields-#39

Conversation

@fizruk

@fizruk fizruk commented Jan 21, 2016

Copy link
Copy Markdown
Member

Closes #39.

This change affects many names and types.
The goal is to make code look nicer.

Here's one example:

--- mempty
---   & schemaTitle   ?~ "Email"
---   & schemaType    .~ SwaggerString
---   & schemaFormat  ?~ "email"
+-- (mempty :: Schema)
+--   & title  ?~ "Email"
+--   & type_  .~ SwaggerString
+--   & format ?~ "email"

These names had to gain _ suffix:

  • type_, in_, default_ (keywords);
  • minimum_, maximum_ (conflict with Prelude);
  • enum_ (conflict with Control.Lens).

@fizruk

fizruk commented Jan 21, 2016

Copy link
Copy Markdown
Member Author

The downside of this change would be more ambiguous types, since names are overloaded now.
However, I can't tell how much a problem that would be.

I still have to check/update the documentation and, probably, write more about lenses and shortcut Has*/Ixed/At instances.

@dmjio, @jkarni I would appreciate you opinion/review on this PR. Note that these changes would affect servant-swagger users as well.

@jkarni

jkarni commented Jan 21, 2016

Copy link
Copy Markdown

This LGTM. It seems a lot nicer to use, and at least for my purposes ambiguity isn't an issue.

One thing to note is that jsonPrefix is a little unsafe - there's no check that the letters dropped are actually the start of the field name. This PR doesn't seem to run afoul of that, but at some point maybe using TH for the From/ToJSON instances so you can safely error out of it doesn't match might be an option.

@fizruk

fizruk commented Jan 21, 2016

Copy link
Copy Markdown
Member Author

@jkarni I have deliberately switched all TH instances to Generic-based in #25.
It had to do with on of the aeson-0.10.0.0 bugs.

Instead of switching back to TH we might improve on the jsonPrefix to automatically detect to drop common prefix.

- use makeFields for records
- remove prefixes even for ParamSchema accessors
- Swagger record field names had to gain "swagger" prefix again
- also remove Paths as a redundant structure
@dmjio

dmjio commented Jan 21, 2016

Copy link
Copy Markdown
Collaborator

👍

@fizruk

fizruk commented Jan 22, 2016

Copy link
Copy Markdown
Member Author

@jkarni @dmjio this is a pretty heavy API change. Should it be released as 2.0 or 1.3?

@dmjio

dmjio commented Jan 22, 2016

Copy link
Copy Markdown
Collaborator

2.0, it's a big change

fizruk added a commit that referenced this pull request Jan 22, 2016
@fizruk
fizruk merged commit 6c0a0ca into master Jan 22, 2016
@fizruk
fizruk deleted the make-fields-#39 branch January 22, 2016 00:54
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.

Consider using makeFields, not makeLenses

3 participants