Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

impl Debug for sc_service::Configuration #6400

Merged
gavofyork merged 9 commits into from
Jun 23, 2020
+139 −59
Merged

impl Debug for sc_service::Configuration #6400

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2dd4f9a
Initial commit
cecton Jun 18, 2020
fb31420
Make sc_service::Configuration derive Debug
cecton Jun 18, 2020
563da0a
Replace task_executor fn's input by proper TaskExecutor type (cleaner)
cecton Jun 18, 2020
7b0590e
impl From<Fn> for TaskExecutor
cecton Jun 18, 2020
d9e6275
Update client/cli/src/runner.rs
bkchr Jun 19, 2020
d53a1c2
Add some doc, examples and tests
cecton Jun 19, 2020
5cb14a0
Replace Deref by fn spawn as suggested
cecton Jun 19, 2020
fc69a32
Merge commit ef2a6c1c047f7afc1587006ddadcf16edf29d402 (no conflict)
cecton Jun 22, 2020
1a67120
Merge commit 41970e7ea56d59d45861d65490a329fe216df74e (conflicts)
cecton Jun 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Replace Deref by fn spawn as suggested
  • Loading branch information
@cecton
cecton committed Jun 19, 2020
commit 5cb14a0d6024b3a11ea9a3aad170cae4557ec93c
9 changes: 4 additions & 5 deletions client/service/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,10 +313,9 @@ where
}
}
Copy link
Contributor Author

@cecton cecton Jun 18, 2020

Choose a reason for hiding this comment

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

This basically makes the struct callable. I'm not sure if this is the best solution.

  1. We could say it's not callable at all and use a function instead
  2. We could implement Fn but I think this is only available in nightly
  3. It might be useful to be able to get the Arc<Fn(future)> from TaskExecutor

Any advice on what would be the cleanest implementation?

Copy link
Contributor

@expenses expenses Jun 18, 2020

Choose a reason for hiding this comment

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

As all the TaskExecutor has to do is to spawn futures and be cloned, I think the best idea would be to hide the internals and just add a spawn method.

Copy link
Contributor

@tomaka tomaka Jun 18, 2020

Choose a reason for hiding this comment

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

While Arc<dyn Fn(Pin<Box<dyn Future<Output = ()> + Send>>, TaskType) + Send + Sync> is not the most the most elegant type, anyone who knows the Rust programming language can understand this type. If you put it in a new TaskExecutor struct, now everyone has to go look at the definition of this TaskExecutor to know how to use it.

I know it's not much, but having so many typedefs and wrapping structs is in my opinion a major pain point in the Substrate code base as whole.

Copy link
Contributor Author

@cecton cecton Jun 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Oh well. My goal was not to wrap it, I just needed to implement Debug and there are only 3 possible ways:

  1. Wrapping that type and derive Debug on the whole Configuration
  2. Writing a complete Debug impl for Configuration and handle special cases separately (more maintenance)
  3. Use a crate that would handle that kind of use case (not sure which one) which will reduce the maintenance but add a dependency (this looks good: https://crates.io/crates/debug_stub_derive)

I think the trade-off is fine here especially with the From because you can simply pass a closure and do .into(). I think it would be better if I document it better so anyone who bumps into this will easily know what to do.

Copy link
Contributor Author

@cecton cecton Jun 19, 2020

Choose a reason for hiding this comment

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

(The other good point is that you don't need to import Arc and Pin, not even TaskExecutor, you just pass the closure and use .into())

Copy link
Contributor

@pscott pscott Jun 19, 2020

Choose a reason for hiding this comment

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

I think adding some documentation would indeed be a good idea :)

Copy link
Contributor Author

@cecton cecton Jun 19, 2020

Choose a reason for hiding this comment

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

done

Copy link
Contributor Author

@cecton cecton Jun 19, 2020

Choose a reason for hiding this comment

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

@expenses done (I just noticed your comment now)


impl std::ops::Deref for TaskExecutor {
type Target = TaskExecutorInner;

fn deref(&self) -> &Self::Target {
&self.0
impl TaskExecutor {
/// Spawns a new asynchronous task.
pub fn spawn(&self, future: Pin<Box<dyn Future<Output = ()> + Send>>, task_type: TaskType) {
self.0(future, task_type)
}
}
2 changes: 1 addition & 1 deletion client/service/src/task_manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ impl SpawnTaskHandle {
}
};

(self.executor)(Box::pin(future), task_type);
self.executor.spawn(Box::pin(future), task_type);
}
}

Expand Down