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

Adding possibility to simple inspect future polling #2

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

riomus
Copy link

@riomus riomus commented Aug 26, 2017

I have added simple inspection future that only logs status and error instead of the whole content of future.

@skade
Copy link
Owner

skade commented Aug 27, 2017

Hi, thanks for the PR :).

I'm not sure I agree with the path of implementation, as it requires to switch out the future in use if you want to choose a different strategy.

There's two ways I see how that could be amended:

  • Make your method the default way for debug! level and have the "old" way of working available under trace! level.
  • Use a feature-flag to switch between the implementation.

I slightly err towards the first, but an obvious problem there is that trace! includes debug!, so every action might be logged twice.

@riomus
Copy link
Author

riomus commented Aug 29, 2017

Hi,

I think that going with 1 option would be good. There could be also an implementation that checks what kind of log level is enabled and then logs appropriately (only once). Ther will be also two implementations because of trait bounds (some items and errors are not implementing Debug)

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