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

Add support for optics #200

Merged
merged 1 commit into from
Nov 10, 2019
Merged

Add support for optics #200

merged 1 commit into from
Nov 10, 2019

Conversation

arybczak
Copy link
Contributor

@arybczak arybczak commented Oct 21, 2019

PR adds support for using optics library instead of lens as an alternative. There are no name clashes because optics supports lenses/prisms as labels.

cc @phadej

EDIT: Travis doesn't pick up insert-ordered-containers from the cabal.project file 🤔

@phadej
Copy link
Collaborator

phadej commented Oct 21, 2019

It doesn't pick, because .travis.yml is generated using swagger2.cabal, not cabal.project (see REGENDATA in generated file, or header).

Anyway, https://hackage.haskell.org/package/insert-ordered-containers-0.2.3 is released

Copy link
Collaborator

@phadej phadej left a comment

Choose a reason for hiding this comment

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

Please bump base >=4.9, so it's not forgotten.

src/Data/Swagger/Optics.hs Outdated Show resolved Hide resolved
src/Data/Swagger.hs Outdated Show resolved Hide resolved
src/Data/Swagger/Optics.hs Outdated Show resolved Hide resolved
@arybczak
Copy link
Contributor Author

Ok, I pushed the changes and it's green now 👍.

Copy link
Collaborator

@phadej phadej left a comment

Choose a reason for hiding this comment

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

@fizruk what you think?

@arybczak
Copy link
Contributor Author

For the record, I'm working on a code base that currently uses fclabels for field accessors, but also uses swagger which requires lens at the moment. We're in the process of replacing fclabels with optics and want to use them everywhere, including swagger specific code because having the same operators be optics-specific and lens-specific in different modules would be super confusing.

@arybczak
Copy link
Contributor Author

I improved haddock documentation and added doctests.

@phadej
Copy link
Collaborator

phadej commented Oct 22, 2019

There's a conflict now Not anymore.

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.

If nobody requests any more changes, I will merge this on ~2019-11-08.

@fisx
Copy link
Collaborator

fisx commented Nov 9, 2019

Trivial conflict, I've resolved it locally and will try to push to master once the tests have passed.

@fisx
Copy link
Collaborator

fisx commented Nov 9, 2019

hum...

$ stack test --fast
[...]
[ 7 of 17] Compiling Data.Swagger.Optics
                        
/home/mf/src/swagger2/src/Data/Swagger/Optics.hs:119:10: error:
    • Couldn't match type ‘Join
                             A_Lens
                             (IxKind
                                (Data.HashMap.Strict.InsOrd.InsOrdHashMap
                                   HttpStatusCode (Referenced Response)))’
                     with ‘An_AffineTraversal’
        arising from a use of ‘%’
    • In the expression: #responses % ix n
      In an equation for ‘ix’: ix n = #responses % ix n
      In the instance declaration for ‘Ixed Responses’
    |                   
119 |   ix n = #responses % ix n
    |          ^^^^^^^^^^^^^^^^^
[...]

my stack.yaml:

resolver: lts-14.3
packages:
- '.'
extra-deps:
- optics-core-0.2
- optics-th-0.2
- indexed-profunctors-0.1

i guess i better don't push... sorry @arybczak, could you take another look?

@arybczak
Copy link
Contributor Author

lts-14.3 has insert-ordered-containers-0.2.2, this PR needs 0.2.3 (as is in .cabal file). Does stack override bound checks from the cabal file?

@fisx
Copy link
Collaborator

fisx commented Nov 10, 2019

lts-14.3 has insert-ordered-containers-0.2.2, this PR needs 0.2.3 (as is in .cabal file). Does stack override bound checks from the cabal file?

oops, yes, the way i configured it. it seems allow-newer implies allow-older-too?

(disclaimer: not a proud stack user, just didn't have a lot of time, and couldn't think of a quicker way to test it.)

@fisx
Copy link
Collaborator

fisx commented Nov 10, 2019

yes, with the extra stack.yaml tweak the test suite passes. i'll merge this to master manually soon!

@arybczak
Copy link
Contributor Author

arybczak commented Nov 10, 2019

oops, yes, the way i configured it. it seems allow-newer implies allow-older-too?

Yes, that seems to be the case: commercialhaskell/stack#4495 (comment)

I updated the branch, I think you can now merge via github 👍

@fisx fisx merged commit c03fdf1 into GetShopTV:master Nov 10, 2019
@fisx
Copy link
Collaborator

fisx commented Nov 10, 2019

Thanks!

@fisx fisx mentioned this pull request Nov 10, 2019
This was referenced Nov 11, 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.

None yet

3 participants