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

rename #[async_worker] to #[ockam::worker] #1220

Closed
mrinalwadhwa opened this issue Apr 10, 2021 · 8 comments
Closed

rename #[async_worker] to #[ockam::worker] #1220

mrinalwadhwa opened this issue Apr 10, 2021 · 8 comments

Comments

@mrinalwadhwa
Copy link
Member

mrinalwadhwa commented Apr 10, 2021

  1. This export would change
    https://github.com/ockam-network/ockam/blob/develop/implementations/rust/ockam/ockam/src/lib.rs#L40

to:

... as worker;
  1. we'll also have to find and replace all use of #[async_worker] with #[ockam::worker]
  2. remove imports of async_worker, #[ockam::worker] will work if ockam is a dependency.
@akshay1992kalbhor
Copy link

Looks like #[async_worker] is mostly used in ockam_examples. Does that sound correct? Also used somewhat in documentation and ockam_transport_tcp. Just making sure I am on the right path

@mrinalwadhwa
Copy link
Member Author

Yes that's right.

@gabhijit
Copy link
Contributor

I am just thinking should the name be kept as it is or just the original name async_trait as async_worker (or proposed worker) is just an alias to async_trait, which may not be obvious to the reader of the code simply looking at that code. And as such that annotation is really only required because of it being async_trait. It's perhaps still a good idea to bring it under ockam:: namespace explicitly.

Also, related nit:

  1. Inside the ockam_channel, original async_trait is used, so for consistency, it should also use ockam::* whatever is decided to be the right choice of name.
  2. Also the ockam_core trait should also use the same name for consistency! It is using async_trait.

There may be other places, where I might have missed it, so perhaps a good idea to define it consistently at one place and use the same name everywhere.

@mrinalwadhwa
Copy link
Member Author

@gabhijit thank you, completely agree on consistency 👍

The reason to rename to ockam::worker is because we suspect we may be able hide more worker definition boilerplate behind it. The associated types, for example, are potential candidates cc: @spacekookie @jared-s

https://github.com/ockam-network/ockam/blob/9a6b2940e814b297376a30b04886dcf19aabd270/examples/rust/get_started/src/echoer.rs#L1-L16

@SanjoDeundiak
Copy link
Member

@gabhijit @mrinalwadhwa In order to use ockam::Worker macro everywhere, it should be moved to ockam_core (or even separate dedicated crate), because many ockam crates need to implement Worker, but they do not depend on ockam crate

@gabhijit
Copy link
Contributor

@SanjoDeundiak (Note: it's a worker and not Worker), Worker is a trait and worker is just alias to async_trait proc-macro. Yes, it can come from ockam_core:: (and can be re-exported from ockam) as long as it is consistently used everywhere.

As such for the boiler plate part, one will have to write another proc-macro ockam::worker say that internally wraps this async_trait and handles associated types. But that appears to be a bit of an overkill at the moment, since the user (implementer of the Worker trait for their own type), will have to know that those are associated types and will have to deal with them anyways.

@mrinalwadhwa
Copy link
Member Author

All good points @gabhijit @SanjoDeundiak.

To elaborate on what I'm imagining ... Line 10 has all the information for line 7 and 8 and my hope is we can eliminate line 7 and 8

https://github.com/ockam-network/ockam/blob/9a6b2940e814b297376a30b04886dcf19aabd270/examples/rust/get_started/src/echoer.rs#L1-L16

@spacekookie
Copy link
Contributor

For the time being I don't think it matters too much where we create the async_trait to worker alias. Although considering that we have lots of crates that create workers and might not be able to depend on the ockam crate (because of circular dependencies), it makes sense to go with @SanjoDeundiak's suggestion and move that alias to ockam_core.

Ultimately we'll need to have a separate crate, similar to how the ockam::node attribute is implemented. Or we rename that crate to ockam_attributes and put both node and worker there.

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 a pull request may close this issue.

5 participants