Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

executive tracing cleanup #903

Merged
merged 7 commits into from Apr 12, 2016
Merged

executive tracing cleanup #903

merged 7 commits into from Apr 12, 2016

Conversation

debris
Copy link
Collaborator

@debris debris commented Apr 8, 2016

  • moved tracing logic to Tracer trait implementations
  • moved result field from TraceAction variants to Trace
  • named fields of the result
  • removed TraceAction::Unknown variant
  • reduced code complexity by reducing number of if / if let statements and moving tracing logic to tracer

// this pr is prelude to db tracing module (more to come tomorrow)

@debris debris added the A0-pleasereview 🤓 Pull request needs code review. label Apr 8, 2016
}

/// This funtion should be used to execute transaction.
pub fn transact(&'a mut self, t: &SignedTransaction, options: TransactOptions) -> Result<Executed, Error> {
let check = options.check_nonce;
match options.tracing {
true => self.transact_with_tracer(t, check, ExecutiveTracer::default()),
Copy link
Contributor

Choose a reason for hiding this comment

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

just pull out the final param into the conditional

perhapes use inline if

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just pull out the final param into the conditional

I wanted, but I can't.

let t = if options.tracing {
  ExecutiveTracer::default()
else {
  NoopTracer
}

This is not valid expression, because of the type difference. And if I Box the tracer, the type information is lost.

@gavofyork gavofyork added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 9, 2016
@gavofyork gavofyork added A0-pleasereview 🤓 Pull request needs code review. A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. A0-pleasereview 🤓 Pull request needs code review. labels Apr 12, 2016
@arkpar arkpar merged commit 6af8e5f into master Apr 12, 2016
@gavofyork gavofyork deleted the tracing branch April 28, 2016 15:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants