elichai / log-derive

A procedural macro for auto logging output of functions
https://docs.rs/log-derive
Apache License 2.0
189 stars 12 forks source link

Add support for async #22

Closed DmitrySamoylov closed 4 years ago

DmitrySamoylov commented 4 years ago

The idea is to place function body inside

Some detals: https://users.rust-lang.org/t/nested-async-block-fn/44560/2

Closes #20

elichai commented 4 years ago

Tested and read on async blocks and this looks great :) 2 questions though:

  1. Why does async needs to be behind a feature?
  2. how do you feel about replacing tokio in the tests with futures_executor::block_on?
DmitrySamoylov commented 4 years ago
  1. There is a check in Travis that we should build under older rust versions (async was introduced in 1.39). Did not touch that part but if we could drop support for older Rust versions then we don't need features.
  2. Yeah that's fine. Any executor will do
elichai commented 4 years ago
  1. There is a check in Travis that we should build under older rust versions (async was introduced in 1.39). Did not touch that part but if we could drop support for older Rust versions then we don't need features.

WIll this line fail to compile on older compilers? https://github.com/elichai/log-derive/pull/22/files#diff-914b24fcd759ea4189775083e2b4434cR9 if it doesn't we can replace it back to my ExprClosure and copy the asyncness of the function.

  1. Yeah that's fine. Any executor will do

Feel free to do it(tokio is really big and I'd like to avoid it even for tests), otherwise I'll merge as is and change it later

DmitrySamoylov commented 4 years ago
  1. Yep, this line will fail to compile. But also tests for async won't compile
  2. Will do it in a moment
elichai commented 4 years ago
  1. Yep, this line will fail to compile. But also tests for async won't compile

I'd rather feature gate tests and not actual logic if possible, most users don't really check features they just use the default. I'll look into maybe using syn::ExprAsync which I think should compile but will fail on the user side if he uses async functions with older compilers

DmitrySamoylov commented 4 years ago

So, the idea is to make an enum with syn::ExprClosure and syn::ExprAsync instead of raw TokenStream? This can work I think. Let me try :)

DmitrySamoylov commented 4 years ago

Think I finished force-pushing :smiley:

elichai commented 4 years ago

This is awesome! I see you also decided to remove the call and added that to the expression itself :) Too bad we need to specify so many default arguments (for a while I'm trying to decide between Default::default() and token![SOMETHING]) one small nit and i'll merge :)

elichai commented 4 years ago

FYI, feel free to open an issue with some feedback about the UX and usability, it's pretty hard to know how people use this because on one hand it has 161 stars, but on the other 0 dependent crates.

elichai commented 4 years ago

Also @silverjam do you mind that the travis commit is now here? would you prefer a cherry-pick to include your contribution, or removing that commit from here and you'll rebase your PR afterwards? :)

DmitrySamoylov commented 4 years ago

Oh, I shouldn't have stolen @silverjam contribution Cherry-picked those commits as a default option for @silverjam :smiley:

DmitrySamoylov commented 4 years ago

FYI, feel free to open an issue with some feedback about the UX and usability, it's pretty hard to know how people use this because on one hand it has 161 stars, but on the other 0 dependent crates.

Just started to use this crate and for now it looks very clean for me. All I could imagine is already here. Will add a feature request if I think of something :)

silverjam commented 4 years ago

Also @silverjam do you mind that the travis commit is now here? would you prefer a cherry-pick to include your contribution, or removing that commit from here and you'll rebase your PR afterwards? :)

Oh, I shouldn't have stolen @silverjam contribution Cherry-picked those commits as a default option for @silverjam

Thanks for keeping the attribution :-), but in this case it's small enough I don't mind either way.

DmitrySamoylov commented 4 years ago

@elichai Could you please bump version?

elichai commented 4 years ago

@elichai Could you please bump version?

Done