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

Introduce strongly-typed Context keys #417

Merged
merged 1 commit into from Jan 15, 2018

Conversation

6 participants
@matklad
Copy link
Contributor

matklad commented Jan 10, 2018

So this was born out of #416 (comment).

The plan to add Context::service_config is not perfect, because context can very well lack service_config at all. I think what we really want here is something like this:

pub trait ServiceFactory: 'static {
    #[allow(unused_variables)]
    fn command(&mut self, command: CommandName) -> Option<Box<CommandExtension>>;
    fn make_service(&mut self, run_context: &Context) -> Box<Service>;
}

pub trait ServiceFactoryWithConfig: ServiceFactory {
    type Config;
    fn make_service(&mut self, run_context: &Context, config: Self::Config) -> Box<Service>;
}

This is probably doable, and even in a nice way, despite the problem of object-safe associated types (which I believe is surmountable in a nice way by this trick).

However, this PR in particular proposes a smaller and more localized fix: we actually can statically type all available Context keys. That is, we, instead of stringly-typed API, can provide users with a set of named constants, which allow to extract config values of particular type.

This PR implements it only for NodeConfig and AbstractConfig, if this is something what we want, I'll convert all other string keys to this pattern.

@@ -191,6 +224,11 @@ impl Context {
}
}

/// Gets the variable from the context.
pub fn get2<'de, T: Deserialize<'de>>(&self, key: Key<T>) -> Result<T, Box<Error>> {

This comment has been minimized.

@matklad

matklad Jan 10, 2018

Author Contributor

Obviously, this will be named get in real implementation. Note the crucial property: when the user supplies some key, they don't need to specify T explicitly, and they can't specify the wrong T even if they want to.

@matklad matklad force-pushed the matklad:strongly-typed-strings branch from 76ca81c to 89efdd2 Jan 10, 2018

@matklad

This comment has been minimized.

Copy link
Contributor Author

matklad commented Jan 10, 2018

Relevant funny image

@matklad matklad force-pushed the matklad:strongly-typed-strings branch from 89efdd2 to 4b4bedf Jan 11, 2018

@matklad matklad changed the title Introduce strongly-typed Context keys WIP: Introduce strongly-typed Context keys Jan 11, 2018

@matklad matklad requested a review from vldm Jan 11, 2018

@matklad

This comment has been minimized.

Copy link
Contributor Author

matklad commented Jan 11, 2018

So this is "production ready" I think, modulo missing doc comments for various configs that we actually use: I haven't wrapped my head around what is the purpose of each key yet :)

@matklad matklad force-pushed the matklad:strongly-typed-strings branch from 4b4bedf to 6058b1d Jan 11, 2018

pub __phantom: PhantomData<T>,
}

// We need explicit `impl Copy` because derive won't work if `T: !Copy`.

This comment has been minimized.

@vitvakatu

vitvakatu Jan 11, 2018

Contributor

I wonder if we can use derivative for similar cases.

This comment has been minimized.

@matklad

matklad Jan 11, 2018

Author Contributor

Hm, interesting... I think this is a very rare case, so I would prefer to avoid pulling in a proc-macro just for this.

@matklad matklad force-pushed the matklad:strongly-typed-strings branch 3 times, most recently from 4c7baaf to 617ee56 Jan 11, 2018

}
}

#[macro_export]

This comment has been minimized.

@slowli

slowli Jan 11, 2018

Contributor

Add documentation before merging, please. For example, the following doctest could reveal that the current macro implementation is not precisely correct:

#[macro_use] extern crate exonum;
use exonum::helpers::fabric::Key as ConfigKey;

fn main() {
    const KEY: ConfigKey<String> = config_key!("foo");
    // Should probably use the key somehow
}

This comment has been minimized.

@matklad

matklad Jan 11, 2018

Author Contributor

Yep, too bad no_missing_docs does not work for macros 🙃

I am also not sure about naming here: the ideal names would be Key and key!, but macros are not namespaced yet, so key! potentially can collide with something else, and that's why I choose to name it config_key instead.

Perhaps this fear is unfounded, and key! would be a fine name? What do you all think?

@matklad matklad force-pushed the matklad:strongly-typed-strings branch 3 times, most recently from 198327c to e0a74e7 Jan 11, 2018

@matklad

This comment has been minimized.

Copy link
Contributor Author

matklad commented Jan 12, 2018

Added minimal docs for the particular keys: they are a bit silly, because I don't yet know which configs mean what, but at least they mention relevant command names :-)

Open question about naming:

  • Key and key! -- optimal names, imo, but key! risks collision
  • ContextKey and context_key! -- longer, but no collision and nodes in the direction of Context.
  • ConfigKey and config_key! -- a slight variation of the previous one.

@matklad matklad changed the title WIP: Introduce strongly-typed Context keys Introduce strongly-typed Context keys Jan 12, 2018

@matklad

This comment has been minimized.

Copy link
Contributor Author

matklad commented Jan 12, 2018

And here's a proof-of-concept implementation of ServiceFactory with associated config, which also is related to the discussion about associated types in transactions: matklad@316c730

@DarkEld3r

This comment has been minimized.

Copy link
Contributor

DarkEld3r commented Jan 12, 2018

Open question about naming:

In C/C++ world where macros also doesn't belong to namespaces it is highly common to use project name as prefix. So it can be exonum_key! or exonum_context_key! (I suppose key! is too generic even if we talk only about our project).

@matklad

This comment has been minimized.

Copy link
Contributor Author

matklad commented Jan 12, 2018

@DarkEld3r that's an interesting idea, but I don't think that this pattern is widespread in Rust, and, if we go with exonum_key!, we probably should use exonum_message! and exonum_encoding_struct! for consistency. I'll switch the names for context_key now, as it seems sufficiently specific, and I suggest we use this name unless someone has strong objections :)

@matklad matklad force-pushed the matklad:strongly-typed-strings branch from e0a74e7 to 05a1b13 Jan 12, 2018

@vldm

vldm approved these changes Jan 15, 2018

@DenisKolodin DenisKolodin merged commit a94c8b8 into exonum:master Jan 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@matklad matklad referenced this pull request May 24, 2018

Merged

Typed Config Access #5552

@guevara guevara referenced this pull request Nov 2, 2018

Open

Typed Key Pattern #1403

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.