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

No record prefixes #844

Merged
merged 19 commits into from Dec 1, 2022
Merged

No record prefixes #844

merged 19 commits into from Dec 1, 2022

Conversation

endgame
Copy link
Collaborator

@endgame endgame commented Nov 24, 2022

Remove all leading underscores and record field prefixes from records in amazonka-core and amazonka. This is a big step towards removing the explicit lens dependency (though we'll leave that until 2.1); instead, people should look to using record updates, generic-lens/generic-optics or (in GHC >=9.2) -XOverloadedRecordDot for working with these fields.

Other breaking changes: some functions were renamed/removed for consistency or to avoid name clashes:

  • Amazonka.Env.envAuthMaybe -> Amazonka.authMaybe (and its re-export from Amazonka)
  • Amazonka.Env.configure -> Amazonka.Env.configureService (and its re-export from Amazonka)
  • Amazonka.Env.override -> Amazonka.Env.overrideService (and its re-export from Amazonka)
  • Amazonka.Env.timeout -> Amazonka.Env.globalTimeout (and its re-export from Amazonka)
  • Amazonka.Env.within: removed; it was merely a record update
  • Amazonka.Body._Body: removed.

Question for discussion: Should we provide van Laarhoven lenses for these records, like we do with the generated records in bindings? I see no problem with doing this; we can eventually go to a profunctors dependency and provide VL optics without too much trouble, though the few optics which use filtered will need a bit of thought.

Also, since we're going to have to regenerate everything, I bumped botocore and configured all services that generated cleanly. A full regeneration is available at branch no-record-prefixes-gen.

Since this touches a bunch of signing code, I would appreciate reports from other people who have a chance to test this branch. Reports from people using presigning are especially welcome.

Closes #778
Fixes #752

@endgame endgame added this to the 2.0 milestone Nov 24, 2022
@endgame
Copy link
Collaborator Author

endgame commented Nov 24, 2022

This PR also changes how overrides are threaded to the defaultService in bindings, so if you use a non-S3 object store (@ivb-supercede @tanyabouman @kfigiela) I would love to hear that it's still working for you.

@ysangkok
Copy link
Contributor

Looking at the generated branch, lenses are still prefixed by their type, is that intended?

E.g. putObject_contentType :: Lens.Lens' PutObject (Prelude.Maybe Prelude.Text)

@endgame
Copy link
Collaborator Author

endgame commented Nov 24, 2022

Yes, absolutely. This PR only touches amazonka-core and amazonka, and there are no plans to change those generated lenses.

@endgame
Copy link
Collaborator Author

endgame commented Nov 24, 2022

But also: should we provide lenses for the un-prefixed records in that style, for consistency with the generated service bindings?

@ysangkok
Copy link
Contributor

We avoid lenses in the code base that we use Amazonka in, so I don't feel strongly about it. But I'd prefer as much consistency as possible between amazonka-core and other packages.

@endgame
Copy link
Collaborator Author

endgame commented Nov 29, 2022

I have rearranged some module names so that Amazonka.Lens can provide optics for all of amazonka and amazonka-core.

@ivb-supercede
Copy link
Contributor

This PR also changes how overrides are threaded to the defaultService in bindings, so if you use a non-S3 object store (@ivb-supercede @tanyabouman @kfigiela) I would love to hear that it's still working for you.

Works fine in my testing, overriding the addressing style functions as expected.

Question for discussion: Should we provide van Laarhoven lenses for these records, like we do with the generated records in bindings?

Please. Unprefixed record fields are a bit of an importer's nightmare, and our codebase also leans heavily on Amazonka lenses.

@endgame
Copy link
Collaborator Author

endgame commented Nov 29, 2022

Please. Unprefixed record fields are a bit of an importer's nightmare, and our codebase also leans heavily on Amazonka lenses.

These are in the PR as of a few hours ago, but I'm curious: have you tried generic-lens or similar? That's what I normally use when writing code that uses amazonka.

@ivb-supercede
Copy link
Contributor

have you tried generic-lens or similar? That's what I normally use when writing code that uses amazonka.

We use these rarely, but Generic is bad for compile times, and I prefer to avoid it. If I want lenses without depending on lens, I normally reach for microlens instead.

The lenses already in this PR are great, I relied on them to migrate our existing code when testing the integration with our non-S3 store.

@endgame
Copy link
Collaborator Author

endgame commented Nov 30, 2022

A couple of other bikeshedding questions, and then I'll probably merge this unless I hear objections:

  1. Should we remove the S3AddressingStyle prefix from the different addressing styles? I'm thinking "keep it", because we don't have tools to deal with ambiguity in case something else wants to call itself Auto
  2. Are there any other types that should have a Generic instance added?

@endgame endgame merged commit e04a895 into brendanhay:main Dec 1, 2022
@tanyabouman
Copy link

I would love to hear that it's still working for you.

Works for us. Thanks!

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.

Remove underscores from amazonka/amazonka-core record fields
4 participants