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

Add Hashmap StateMachine #73

Merged
merged 10 commits into from Jul 30, 2015

Conversation

Projects
None yet
3 participants
@Hoverbear
Copy link
Owner

Hoverbear commented Jul 25, 2015

Could be preliminarily merged so it can be used for the changes we talked about with the Register StateMachine as well but still needs more work!

/// A state machine that holds a single mutable value.
#[derive(Debug)]
pub struct HashmapStateMachine {
map: HashMap<String, Value>,

This comment has been minimized.

@danburkert

danburkert Jul 26, 2015

Contributor

Maybe I'm missing the point of this, but why are you constraining the key and value types? It seems like both should be generic and bounded by Serialize and Deserialize.

This comment has been minimized.

@Hoverbear

Hoverbear Jul 26, 2015

Owner

Yes, I agree!

@@ -0,0 +1,62 @@
#![cfg(feature="hashmap")]

This comment has been minimized.

@danburkert

danburkert Jul 26, 2015

Contributor

I think maybe this is unnecessary since it's on the module declaration in state_machine/mod.rs.

This comment has been minimized.

@Hoverbear

Hoverbear Jul 26, 2015

Owner

It may be... Without it though how would the reader know this was only loaded on the feature?

@@ -13,6 +13,7 @@ use std::fmt::Debug;
mod channel;
mod null;
mod register;
#[cfg(feature="hashmap")] mod hashmap;

This comment has been minimized.

@danburkert

danburkert Jul 26, 2015

Contributor

this either needs to be pub mod, or reexported as the ones below are for anyone to use it.

@danburkert

This comment has been minimized.

Copy link
Contributor

danburkert commented Jul 26, 2015

Would it be possible to put this entirely in the examples/ directory? Ideally we would do that for all StateMachine implementations that aren't used in unit tests. This probably includes the register one too, I don't think we are currently using that.

@Hoverbear

This comment has been minimized.

Copy link
Owner

Hoverbear commented Jul 26, 2015

@danburkert I put this here on the precidence of the register example, but I do agree. I'll move the register example and do this as well.

Hoverbear added some commits Jul 26, 2015

@Hoverbear

This comment has been minimized.

Copy link
Owner

Hoverbear commented Jul 26, 2015

@danburkert These examples need some documentation and some cleaning but they're functional. I changed their interface a bit to make it easier to test via scripts.

I do agree that we should probably make state machine not return results and include some help on dealing with errors.

@Hoverbear

This comment has been minimized.

Copy link
Owner

Hoverbear commented Jul 29, 2015

Removed the Result<_,_> from StateMachine. I'd like to set up snapshotting on the examples but then should be ready for r+.

Last tests have been failing due to a dependency. We'll need to re-fire the build once it's fixed.

@Hoverbear

This comment has been minimized.

Copy link
Owner

Hoverbear commented Jul 30, 2015

@homu r=danburkert (Via IRC)

@homu

This comment has been minimized.

Copy link
Collaborator

homu commented Jul 30, 2015

📌 Commit dc5f017 has been approved by danburkert

homu added a commit that referenced this pull request Jul 30, 2015

Auto merge of #73 - Hoverbear:hashmap, r=danburkert
Add Hashmap StateMachine

Could be preliminarily merged so it can be used for the changes we talked about with the Register StateMachine as well but still needs more work!
@homu

This comment has been minimized.

Copy link
Collaborator

homu commented Jul 30, 2015

⌛️ Testing commit dc5f017 with merge 959bd95...

@Hoverbear Hoverbear added this to the 0.0.1 milestone Jul 30, 2015

@homu

This comment has been minimized.

Copy link
Collaborator

homu commented Jul 30, 2015

☀️ Test successful - status

@homu homu merged commit dc5f017 into master Jul 30, 2015

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls First build on master at 84.335%
Details
homu Test successful
Details

@Hoverbear Hoverbear deleted the hashmap branch Jul 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment