dolittle / Home

Dolittle is a platform designed to build Line of Business applications without sacrificing architectural quality, code quality or scalability.
http://www.dolittle.io
MIT License
27 stars 5 forks source link

Change Reason.Create to enforce a Guid id #75

Open ghost opened 4 years ago

ghost commented 4 years ago

The current .Create accepts any string and crashes run-time. Let's use the compiler to remove a potential run-time error.

If we need it to be a Guid we should not allow strings, even if that makes the method slightly easier to use. It does so at the expense of making run-time errors more likely.

┆Issue is synchronized with this Asana task

ghost commented 4 years ago

➤ Einar Ingebrigtsen commented:

Great point @TomasEkeli - what if we went one further and actually use a ConceptAs called ReasonId. We could then have have an implicit operator on that that parsed the string and actually have the code passing in a string and the parsing would happen implicitly and fail in the same way.

ghost commented 4 years ago

➤ Tomas Ekeli commented:

What I want to avoid is that code that looks fine and builds fine crashes at run-time

Example: Reason.Create("something not a guid", "A good reason", "description");

If we had a ReasonId : ConceptAs with an implicit operator that does Guid.Parse we would still let this slip past the compiler and turn into a runtime exception.

Can we, perhaps, have strings as Ids instead of Guids?

ghost commented 4 years ago

➤ Einar Ingebrigtsen commented:

new Guid() or Guid.Parse() will not evaluate at build time, so you still have the same problem. It will only be seen at runtime no matter what you do for that.

If we want to evaluate this at build time, we could create a Roslyn extension that does the check.

I'm not too keen on having it as a string, even though that would work in isolation. The reason is that we're looking at including rules, broken rules and reasons as part of the MetaModel and actually promoting Rules, BrokenRules and Reasons into Artifacts - which would then require them to be Guids.

ghost commented 4 years ago

➤ Einar Ingebrigtsen commented:

I will move this into Triage for prioritization for now and as a non-breaking change we'll add the ConceptAs<> for this, building on your Pull Request.