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

mtl-2.3 compatibility #779

Merged
merged 3 commits into from
Jun 27, 2022
Merged

mtl-2.3 compatibility #779

merged 3 commits into from
Jun 27, 2022

Conversation

ysangkok
Copy link
Contributor

Before this PR, compilation on mtl-2.3 fails with

src/Amazonka/Error.hs:98:51: error:
    • No instance for (Alternative (Either String))
        arising from a use of ‘<|>’
    • In the third argument of ‘either’, namely
        ‘(h .# hAMZRequestId <|> h .# hAMZNRequestId)’
      In the expression:
        either
          (const Nothing) Just (h .# hAMZRequestId <|> h .# hAMZNRequestId)
      In an equation for ‘getRequestId’:
          getRequestId h
            = either
                (const Nothing) Just (h .# hAMZRequestId <|> h .# hAMZNRequestId)
   |
98 |   either (const Nothing) Just (h .# hAMZRequestId <|> h .# hAMZNRequestId)

The instance Alternative (Either String) makes little sense to me since I don't know how the instance can manufacture an empty value for the Alternative instance.

So in this PR, I try to avoid this instance.

This is untested, I'd appreciate advice on how best to test this.

Copy link
Collaborator

@endgame endgame left a comment

Choose a reason for hiding this comment

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

Thanks for this. I have some requests, beyond the inline comments:

  • Please add mtl >=2.2 && <2.4 to the bounds on amazonka-core.cabal.
  • Please add an entry to lib/amazonka/CHANGELOG.md, which represents the project as a whole.

As for testing, the best thing I can think of is to build some packages (or maybe some of the example programs) with --constraint 'mtl >=2.3' and see if it looks good.

lib/amazonka-core/src/Amazonka/Error.hs Outdated Show resolved Hide resolved
lib/amazonka-core/src/Amazonka/Error.hs Outdated Show resolved Hide resolved
lib/amazonka-core/src/Amazonka/Error.hs Outdated Show resolved Hide resolved
Copy link
Collaborator

@endgame endgame left a comment

Choose a reason for hiding this comment

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

I have another request. How goes your plans to test this?

lib/amazonka-core/src/Amazonka/Prelude.hs Outdated Show resolved Hide resolved
@endgame endgame added this to the 2.0 milestone May 31, 2022
@ysangkok
Copy link
Contributor Author

Testing is going well, cabal test --constraint='mtl>=2.3' is passing with no issues. And I can build the examples.

Copy link
Collaborator

@endgame endgame left a comment

Choose a reason for hiding this comment

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

I am happy with this. Ping me when you're satisfied with your testing and I will merge. Thanks very much.

@endgame
Copy link
Collaborator

endgame commented Jun 6, 2022

@ysangkok is there anything else you want to test before I merge?

@ysangkok
Copy link
Contributor Author

ysangkok commented Jun 6, 2022

@endgame I was hoping that somebody would get the chance to test in a 'staging' first. Sadly at my work we don't have that infrastructure for the project we use Amazonka for, so I would have to test it in production, which is kinda dangerous. I think it looks fairly safe since the worst case seems to be that it passes a Nothing to serviceError, in place of making all of go a Nothing, which doesn't sound too dangerous, but I haven't looked into it.

@endgame endgame merged commit be7688e into brendanhay:main Jun 27, 2022
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.

2 participants