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.

Sign up for GitHub

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

Jump to bottom

spawn-ready: Remove useless MakeSpawnReady type #536

Merged
merged 7 commits into from
Feb 17, 2022
Merged

spawn-ready: Remove useless MakeSpawnReady type #536

merged 7 commits into from
Feb 17, 2022

Conversation

olix0r
Copy link
Collaborator

@olix0r olix0r commented Jan 19, 2021 •
edited
Loading

The spawn_ready module has two notable (public API) problems:

  1. MakeSpawnReady is useless, as it doesn't use the target type in any
    novel way. It's nothing more than a MapResponse.
  2. The SpawnReadyLayer type produces a MakeSpawnReady (which is, as
    mentioned, useless).

This change removes the spawn_ready::make module and modifies
SpawnReadyLayer to produce SpawnReady instances directly.

@olix0r olix0r requested a review from hawkw January 19, 2021 08:06
@olix0r olix0r force-pushed the ver/spawn-ready-simplify branch 2 times, most recently from e1d152a to 3a9b831 Compare January 19, 2021 08:10
@olix0r olix0r marked this pull request as draft January 19, 2021 15:47
@olix0r olix0r changed the title spawn-ready: Simplify the spawn_ready module spawn-ready: Remove useless MakeSpawnReady type Jan 19, 2021
@olix0r
Copy link
Collaborator Author

olix0r commented Jan 19, 2021

This PR depends on #538

@olix0r
spawn-ready: Remove useless MakeSpawnReady type
1539674 
The `spawn_ready` module has two notable (public API) problems:

1. `MakeSpawnReady` is useless, as it doesn't use the target type in any
   novel way. It's nothing more than a `MapResponse`.
2. The `SpawnReadyLayer` type produces a `MakeSpawnReady` (which is, as
   mentioned, useless).

This change removes the `spawn_ready::make` module and modifies
`SpawnReadyLayer` to produce `SpawnReady` instances directly.
@olix0r olix0r marked this pull request as ready for review January 19, 2021 18:14
@olix0r
Copy link
Collaborator Author

olix0r commented Jan 19, 2021

Again, this is a breaking change; but I wanted to open this for discussion. I'm responsible for the current mess and would love to clean it up eventually.

@olix0r olix0r mentioned this pull request Jan 19, 2021
Merged
@hawkw
Copy link
Member

hawkw commented Jan 20, 2021

the current state of SpawnReadyLayer is so convoluted that I can't imagine anyone is actually using it

Could be worth searching GitHub to see if it shows up in the wild...this wouldn't give us a correct negative signal, as there might be any number of private projects or projects on other git hosts using it, but we could at least get a positive signal if it turns out someone is using this...

@davidpdrsn
Copy link
Member

Not quite sure what your plan is but gut my feeling is that we shouldn't be afraid of bumping the version to 0.5 even if this is the only breaking change.

@olix0r
Copy link
Collaborator Author

olix0r commented Jan 20, 2021

From a quick search, there appear to be no uses outside of this repo: https://github.com/search?q=SpawnReadyLayer&type=code

Though, I'm fine with bumping to 0.5 in this PR? (Should that happen in this PR or a distinct followup?)

davidpdrsn
davidpdrsn approved these changes Jan 20, 2021
View reviewed changes
Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

I'm not very familiar with this area of the code but I think this looks correct

@olix0r
Copy link
Collaborator Author

olix0r commented Jan 21, 2021

@hawkw ptal

also: should i bump version to 0.5 in this PR or should that be done when prepping the release?

@hawkw
Copy link
Member

hawkw commented Jan 21, 2021

I haven't looked too closely at the actual change & its rationale yet, but I did want to weigh in on the breaking change: I think that we actually can afford to bump versions for the tower crate fairly liberally, as long as we don't break tower-service or tower-layer. Those crates contain the traits that are necessary for compatibility, while tower itself doesn't. Thus, few other libraries expose tower itself (rather than tower-layer or tower-service) in their public APIs, and in many cases, it should be okay (though not ideal) to have multiple versions of tower in a dependency tree.

That said, I think we should try to get approval from everyone before shipping a breaking change --- at least, it would be nice to get a +1 from @LucioFranco as well. I'd also prefer to do a deprecation for MakeSpawnReady in an 0.4 release before we remove it in 0.5.

@olix0r
Copy link
Collaborator Author

olix0r commented Jan 21, 2021 •
edited
Loading

I'd also prefer to do a deprecation for MakeSpawnReady in an 0.4 release before we remove it in 0.5.

I don't know how we do this gracefully when the intent is to change the signature of SpawnReadyLayer...

Also, I don't really think it's important to do a deprecation cycle as we have no evidence it's actually used anywhere.

@hawkw
Copy link
Member

hawkw commented Jan 21, 2021

I don't know how we do this gracefully when the intent is to change the signature of SpawnReadyLayer...

this is what i get before weighing in before actually reading the change :P

Also, I don't really think it's important to do a deprecation cycle as we have no evidence it's actually used anywhere.

I don't think it's important either :) I just thought that, from the PR title, we were just doing a simple removal, so it seemed like we could do a deprecation cycle quite easily, and we may as well do one. My bad!

hawkw
hawkw approved these changes Jan 21, 2021
View reviewed changes
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

I'm on board with this provided that @LucioFranco (and maybe also @jonhoo?) agree that shipping an 0.5 is reasonable.

Also, we may want to wait to do a quick pass to make sure there's nothing else that ought to be in a breaking release before publishing.

tower/src/spawn_ready/service.rs Show resolved Hide resolved
@olix0r olix0r added this to the 0.5 milestone Jan 22, 2021
@olix0r
Copy link
Collaborator Author

olix0r commented Jan 22, 2021

Per discussion with @LucioFranco: It would be great if we didn't do a breaking release with only such a minor change. So, I've marked this in https://github.com/tower-rs/tower/milestone/3 so we can bundle a set of breaking changes together.

@hawkw
Copy link
Member

hawkw commented Feb 17, 2022

now that we're prepping v0.5, let's get this merged!

@hawkw hawkw enabled auto-merge (squash) February 17, 2022 20:05
@hawkw
Copy link
Member

hawkw commented Feb 17, 2022

@olix0r looks like this has some conflicts that will need to be resolved manually now...

@olix0r
Copy link
Collaborator Author

olix0r commented Feb 17, 2022

@hawkw merged master

@hawkw hawkw merged commit f0e9995 into tower-rs:master Feb 17, 2022
@olix0r olix0r deleted the ver/spawn-ready-simplify branch February 17, 2022 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hawkw hawkw hawkw approved these changes

@davidpdrsn davidpdrsn davidpdrsn approved these changes

@LucioFranco LucioFranco Awaiting requested review from LucioFranco

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.5
Development

Successfully merging this pull request may close these issues.
3 participants
@olix0r @hawkw @davidpdrsn