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

Switch to classy field lenses #41

Merged
merged 6 commits into from
Jan 22, 2016
Merged

Switch to classy field lenses #41

merged 6 commits into from
Jan 22, 2016

Conversation

fizruk
Copy link
Member

@fizruk fizruk commented Jan 21, 2016

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
Copy link
Member Author

fizruk commented Jan 21, 2016

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
Copy link

jkarni commented Jan 21, 2016

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
Copy link
Member Author

fizruk commented Jan 21, 2016

@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
Copy link
Collaborator

dmjio commented Jan 21, 2016

👍

@fizruk
Copy link
Member Author

fizruk commented Jan 22, 2016

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

@dmjio
Copy link
Collaborator

dmjio commented Jan 22, 2016

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