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

Remove uncessary type parameter and constraints from Upgrade service #2436

Merged
merged 5 commits into from
Mar 8, 2023

Conversation

hlbarber
Copy link
Contributor

@hlbarber hlbarber commented Mar 7, 2023

Motivation and Context

The type parameter B was not only redundant but also removed a degree of freedom from the API.

Description

  • Remove type parameter B from Upgrade and related code.
  • Remove repeated constraint on the upgraded service.

@hlbarber hlbarber added breaking-change This will require a breaking change server Rust server SDK labels Mar 7, 2023
@github-actions
Copy link

github-actions bot commented Mar 7, 2023

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python

A new doc preview is ready to view.

@hlbarber hlbarber marked this pull request as ready for review March 7, 2023 17:03
@hlbarber hlbarber requested a review from a team as a code owner March 7, 2023 17:03
@hlbarber hlbarber requested review from weihanglo and jjant and removed request for a team March 7, 2023 17:03
Copy link
Contributor

@jjant jjant left a comment

Choose a reason for hiding this comment

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

  • Why was this type variable there in the first place?
  • Will we be removing any of the other phantom type variables in Upgrade later?

You might want to update these docs (they seem to have an outdated version of Upgrade with only 3 tyvars).

@hlbarber
Copy link
Contributor Author

hlbarber commented Mar 7, 2023

Why was this type variable there in the first place?

Just a mistake - I'm guessing I was mucking around with types and it was required in a previous iteration.

Will we be removing any of the other phantom type variables in Upgrade later?

It's a possibility, this PR was motivated by a specific excess constraint. A more general investigation can be performed and might reveal some extra redundancy.

You might want to update these docs (they seem to have an outdated version of Upgrade with only 3 tyvars).

This was intentional to simplify the documentation. There do seems to be some references to B here that can be removed though - I'll update it.

@hlbarber hlbarber changed the base branch from harryb/long/type-simplification to main March 8, 2023 11:01
@github-actions
Copy link

github-actions bot commented Mar 8, 2023

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python

A new doc preview is ready to view.

@hlbarber hlbarber requested a review from a team as a code owner March 8, 2023 14:01
@github-actions
Copy link

github-actions bot commented Mar 8, 2023

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python

A new doc preview is ready to view.

@hlbarber hlbarber added this pull request to the merge queue Mar 8, 2023
Merged via the queue into main with commit bec93c8 Mar 8, 2023
@hlbarber hlbarber deleted the harryb/remove-b branch March 8, 2023 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This will require a breaking change server Rust server SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants