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

do not force MonadIO on presign #885

Merged
merged 3 commits into from
Jan 18, 2023

Conversation

avieth
Copy link
Contributor

@avieth avieth commented Jan 12, 2023

No description provided.

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.

Since withAuth still has to have a MonadIO constraint, I'm curious what this is buying you. I see it's coming from a workplace fork, which means it's proven necessary for something, so I'm happy to merge it with a couple of tweaks.

In addition to the suggestions inline, there's a couple more changes I'll need to see before I can merge it, but those are minor:

  • Add an entry to lib/amazonka/CHANGELOG.md
  • Re-export withAuth from Amazonka.Auth (as a general rule, most normal users should not have to depend directly on amazonka-core)

lib/amazonka/src/Amazonka/Presign.hs Outdated Show resolved Hide resolved
lib/amazonka/src/Amazonka/Presign.hs Outdated Show resolved Hide resolved
lib/amazonka/src/Amazonka/Presign.hs Outdated Show resolved Hide resolved
@avieth
Copy link
Contributor Author

avieth commented Jan 17, 2023

Since withAuth still has to have a MonadIO constraint, I'm curious what this is buying you. I see it's coming from a workplace fork, which means it's proven necessary for something, so I'm happy to merge it with a couple of tweaks.

In our application we always make an AuthEnv available by other means, never using the Ref constructor of Auth. So this patch just buys us more flexibility/predictability: it's better to not have IO in the type since we never do any. The old definitions which may use IO can all be defined in terms of these pure alternatives, so it seems like a better factoring.

I'll make the suggested changes.

@endgame
Copy link
Collaborator

endgame commented Jan 18, 2023

I agree that it's a better factoring, and I hope we can merge it before I tag RC2.

@avieth
Copy link
Contributor Author

avieth commented Jan 18, 2023

Should be good to go now. LMK if I missed anything.

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.

Couple of final nits, but I look forward to merging this today if you have time to make the changes.

lib/amazonka/src/Amazonka/Presign.hs Outdated Show resolved Hide resolved
lib/amazonka/src/Amazonka/Presign.hs 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.

Thank you very much.

@endgame endgame merged commit 7b8c332 into brendanhay:main Jan 18, 2023
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

2 participants