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

Improve EngineBuilder error message #79

Closed
goblinfactory opened this issue Jan 19, 2020 · 15 comments
Closed

Improve EngineBuilder error message #79

goblinfactory opened this issue Jan 19, 2020 · 15 comments
Assignees

Comments

@goblinfactory
Copy link
Collaborator

goblinfactory commented Jan 19, 2020

Improve the StorageProvider error message so that when a user only references Memstate and follows the getting started guide that the error message will help the user fall into the pit of success i.e. tells the user what to do to resolve the error.

Currently if you only reference memstate and don't include a serializer package, you will get this error after calling Engine.Build ..

Screen Shot 2020-01-19 at 15 23 34

It would be much more helpful if the error message, in addition to the current message could include a message saying something like

It looks like you may be missing one of the default serializers. You may need to add a reference to Memstate.Wire, Memstate.JsonNet, Memstate.Postgres or Memstate.Eventstor, or implement your own serializer and include that. For more help see [link to docs]

The full list can be automatically generated by the StorageProviders class.

@goblinfactory goblinfactory self-assigned this Jan 19, 2020
@goblinfactory goblinfactory changed the title Improve StorageProvider error message Improve EngineBuilder error message Jan 19, 2020
@rofr
Copy link
Member

rofr commented Jan 19, 2020

Good call. I think BinarySerializer is back in some netstandard version, also the new System.Json
If we choose one of these as defaults, then we eliminate this problem altogether. Of course, we need to implement them in the core Memstate nuget. What do you think?

@goblinfactory
Copy link
Collaborator Author

sounds great, worth a spike branch to test?

@rofr
Copy link
Member

rofr commented Jan 19, 2020

As long as we are on netstandard <=2.1 an external library is necessary for System.Text.Json
https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-overview

I'm reluctant to adding it, any other opinions?

@goblinfactory
Copy link
Collaborator Author

I've not dived deep into system.text.json vs newtonsoft.json.
At a quick glance Newtonsoft on .netstandard 2.0 appears to have no dependancies? Compared to a lot more with System.Text.Json.
I'd be leaning towards Newtonsoft.Json, for now, just because memstate user base (well, everyone) is more familiar with Newtonsoft, and later we can upgrade to System.Text.Json if there's a need to?
Screen Shot 2020-01-19 at 22 13 35
both require an external package, though most folk see Newtonsoft.Json as really simply saying you have a dependency on doing Json things :D like floats or decimals.

@goblinfactory
Copy link
Collaborator Author

goblinfactory commented Jan 19, 2020

we should err on what will be easier for us to support moving forward and should be able to swap it out later if needed. If we start working aggresively on improving memstate then we should not be afraid to bring up new major versions as and when it makes sense. i.e. this decision is just a temporary one.

@goblinfactory
Copy link
Collaborator Author

Also, I think it's easily a reversible decision?

@rofr
Copy link
Member

rofr commented Jan 19, 2020

BinaryFormatter is netstandard 2.0 so we could use that as default without introducing dependencies.

The downside is it needs the Serializable attribute on all types.

Should we make it default or move the Newtonsoft.Json implementation into the core lib and make that the default?

@rofr
Copy link
Member

rofr commented Jan 19, 2020

Definitely Newtonsoft over System.Text.Json given all those dependencies. Yikes!

@goblinfactory
Copy link
Collaborator Author

Ah, just had a thought, ...snap, you just ruined my idea, saying it needs the Serializable atttribute on all types... shudder, and .. yuk.

I was going to say, if we include BinaryFormatter out of the box by default, we can always have a 3 lines "how to" guide that we can take a new user through the process of configuring memstate to use a Json Serializer, as part of the getting started.

@goblinfactory
Copy link
Collaborator Author

@goblinfactory
Copy link
Collaborator Author

goblinfactory commented Jan 19, 2020

Why don't we make wire default? Nice and performant, and allows users to follow the getting started to optionally bring in either JsonNet or System.Text.Json depending on their preferences, and both are ridiculously easy. boom... no [Serializable]'s? Not too familiar with wire. And Include BinaryFormatter in the "next steps", then have a link to "advanced" which takes you to postgres and AzureTable Storage. I need AzureTable storage for myself, then plan has been to write one for a long time. ...life!

@rofr
Copy link
Member

rofr commented Jan 19, 2020

Hadn't considered that option actually, but why not.. Wire is by @rogeralsing (akka.net and protoactor wizard) so pretty solid but guessing it's not actively maintained. Only a single dependency and uncommon so not likely to cause conflict with users other deps. A human-readable format would be nice though...

@goblinfactory
Copy link
Collaborator Author

protobuf rulez!

@goblinfactory
Copy link
Collaborator Author

The "human readable" is the "reward" you get for sticking with the getting started guide and writing another 3 lines of code :D
it's all totally up to you (the dev) what you use.

@goblinfactory
Copy link
Collaborator Author

also, cof cof... won't hurt that the default out of the box is just a little quick, and compact. In case folk evaluate tools by looking at the default setup.

rofr pushed a commit that referenced this issue Jan 20, 2020
* test, and fix

* undo unused change

* neaten up hint message

* not using list of candidates in message any more

* shorten test name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants