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

feat(simple): remove Default & 'static requirement with async-scoped feature #13

Closed
wants to merge 6 commits into from

Conversation

beckend
Copy link
Contributor

@beckend beckend commented Jan 28, 2024

#12

  • This now means that Default is a breaking change if they relied on:
    AsyncDropper::default(), Default::default(), #[derive(Default)].
    It's a trade off where you can use any Struct without Default but lose the convenience with the above. If the user as a big struct, this crate becomes a hassle to use, with this change, they have to use new or with_timeout and not rely on Default and having no need to define it.
  • Timeout fixed and tested.
  • 'static lifetime requirement is gone.
  • inner(), inner_mut(), Deref, DerefMut available and tested.
  • Derive #[derive(Debug, PartialEq, Eq, Hash, Clone)] If T allows, easier to store this struct in other structs.

Copy link
Owner

@t3hmrman t3hmrman 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 putting out this PR @beckend 🙇

The code looks great, and I certainly learned about async-scoped today!

One non-code thing -- would you mind using Conventional Commits for your commit message? Something like feat(simple): remove Default & 'static requirement with async-scoped feature would probably represent this well (feel free to adjust that)

Cargo.toml Show resolved Hide resolved
crates/macros_internal/src/lib.rs Outdated Show resolved Hide resolved
crates/async-dropper-simple/src/lib.rs Outdated Show resolved Hide resolved
crates/async-dropper-simple/src/lib.rs Show resolved Hide resolved
crates/async-dropper-simple/Cargo.toml Show resolved Hide resolved
@beckend
Copy link
Contributor Author

beckend commented Jan 29, 2024

I personally copied the code as my own module, I also have 2 versions. This one and one with Default intact.
Honestly I am not going to use this module, I am using my own version.

I did this PR as a suggestion, it's fully open to be modified by you before merge.
It's your library, I think just pick out things you like or not, this is merely a suggestion of code.

@beckend beckend changed the title Refactor tokio simple: No 'static and Default requirement. Fix Timeout feat(simple): remove Default & 'static requirement with async-scoped feature Jan 29, 2024
@beckend
Copy link
Contributor Author

beckend commented Jan 29, 2024

Make your decision and I can adjust. Personally I think that static lifetime should not be required as demonstrated in the comments and tests.

@t3hmrman
Copy link
Owner

Thanks for implementing the changes -- going to take another look and figure out how to merge this work in -- thank you!

@t3hmrman
Copy link
Owner

t3hmrman commented Jan 31, 2024

Hey @beckend I figured out how I'm going to merge in these changes -- I've split up the fixes into separate PRs:

This makes them a bit easier for me to understand personally -- the only one I haven't implemented yet is the non-default bounds, should have that PR upfairly soon as well.

I've put you as a co-author on all the commits, thank you again for the improvements!

@t3hmrman
Copy link
Owner

Closing out this PR since the changes that were in here have made it into 0.2.6:

https://github.com/t3hmrman/async-dropper/releases/tag/async-dropper-simple-v0.2.6

@t3hmrman t3hmrman closed this Jan 31, 2024
@beckend beckend deleted the refactor_tokio_simple branch January 31, 2024 08:28
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