Command history

[mjgpy3]

History + the feature we were seeing yesterday (navigate to an empty command). This probably isn't ready to be merged yet as I realized there's nothing handling adding "unknown commands" to CommandHistory.

[mjgpy3]

@Kazark, what about something like this?

`ModeService` -UnknownCommandEntered text---> `ModeService`
                                          +-> `CommandHistory`

`ModeService` would respond by changing to Normal Mode
`CommandHistory` would save off the text

[Kazark]

@mjgpy3 Well, this is not just an unknown command; it includes all parse errors. Have you looked at the Error type and Errors module in UserNotificationTypes.fs? The current pattern is, if there is an error, publish one of those. That might be a pattern that needs to change, but if so we need to understand that we are changing it, and why. Or is it better to follow the pattern? For example, what if the parser returned not only a string description of what failed, but also the string it was parsing when it failed?

[Kazark]

Also, the stuff with switching to Normal mode is already implemented in ModeService. If you think that's the wrong place, we can discuss moving it to CommandService. Honestly, though, it doesn't feel like that should be impacted by command history stuff.

[mjgpy3]

I see your point. I'll dwell on it some more tonight :)

[Kazark]

Yeah, by the way, how much exposure have you had to event-driven architectures? I might be jumping into the middle of things here assuming we both have the same understanding of command and event messages. We should probably step back and make sure we are philosophically aligned before discussing implementation details. My bad.

[Kazark]

Overall this PR looks great. So glad to have you on the project!!

[mjgpy3]

I had the wrong type in a test. Fixed it, however, I can't seem to run tests on my machine. Are there magic steps to get those running?

[mjgpy3]

Ah, so that's what broke the build. "I don't always compile my code, but when I do, I compile it on the build server." :stuck_out_tongue_winking_eye:

Haha, exactly!

[Kazark]

You will need to install some sort of NUnit runner. I think there is a plugin for Visual Studio. There are also standalone GUI and console runners for it.

[Kazark]

So is this ready to merge, then?

[mjgpy3]

As far as I'm concerned, yes. There will need to be later issues for things like "persistent history"

[Kazark]

I'm happy to merge this now. My only question is—and this is a question, not a requirement—do you care to add any tests before I merge merge merge?

[mjgpy3]

I think I would like to add some

[Kazark]

Okay. Let me know if you need help with that.

[mjgpy3]

Alright, will do!

[Kazark]

I think that is good. I have been relying a lot on manually regressioning stuff before merging, but that is not scalable.

[mjgpy3]

I added some tests, nothing exhaustive. These are very very open to suggestions. They feel kind of bulky for testing so little.

[mjgpy3]

I like it! I was trying to avoid modifying the internal history state, however, I guess knowledge of it is inherently external (a la CommandHistory.Service). I'll give the "modify history" approach a shot!

I am using NCrunch. I like it so far. I'm having some pain seeing why things are failing but I think that's an NUnit thing.

[Kazark]

Haha, that's the way! Think like Git: just rebase everything! ;)

Yeah, actually one of the things that drove me to design the services the way I did was for testability. One of the things Joel taught me was that the only way to make a state machine testable is to inject the state.

I do want other services to treat the state as private, but I don't think it's too much of a liability to make the unit tests depend on the details a little bit. At least at that point you have at max two places to update something, rather than having it scattered throughout the code.

[Kazark]

Looks good. Thanks so much, Mike! This resolves #35

[mjgpy3]

Thank you! This was fun. I'll have to hop on another one soon :+1: