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

Make consistent singlar/plural names of instances to avoid downstream… #193

Merged
merged 21 commits into from
Aug 13, 2021

Conversation

alexreich
Copy link
Contributor

@alexreich alexreich commented Jul 26, 2021

… ambiguous reference issues

Convention is singular for instance and plural for lists and collections, to avoid code such as the following:

public DbSet<WorkflowRules> WorkflowRules { get; set; }

into

public DbSet<Workflow> Workflows { get; set; }

  • WorkflowRules

is now singular:

  • Workflow

@David-Moreira
Copy link
Contributor

David-Moreira commented Jul 26, 2021

Giving my personal opinion.

This is of course always subject to individuals opinions...
I would agree with Alex except for ReSettings, I would say the convention generally is to keep plural. It's not an entity per say, but a bundle of configurations or options, which make sense to be plural as it is.

To put into perspective, when you are dealing with

  • a WorkfkowRule object you are dealing with a single instance, which is considered an "entity" or "model" and you may have a List of them. Makes sense to be singular... As when you instantiate you are dealing with a singular 'entity'
  • a ReSettings object you are dealing with a "bundle" of configurations and generally you do not have a list of these, so plural makes sense here as it clearly explains that when you instantiate the object, it is a bunch of settings.

Also I'm guessing there must be some Microsoft guidelines for these somewhere... Since this is Microsoft endorsed... Might as well take a look at it.

@alexreich
Copy link
Contributor Author

alexreich commented Jul 26, 2021

Agreed with @David-Moreira and reverted ReSettings change since this is an internal object anyway.

Also, found some text from one of the efcore contributors here. And more context in this thread which is the crux of the argument:

Well it's considered a bad practice because a table already implies that multiple entites exist because it has multiple rows, so pluralizing the entity name is redundant. It results in generated key names like FK_Users_Accounts which reads better as FK_User_Account, ie. "foreign key linking a user to an account".

@abbasc52
Copy link
Contributor

abbasc52 commented Jul 28, 2021

Hi @alexreich,

I would like to share my opinion on the naming

  • WorkflowRules - I do agree it is a little misleading but so would WorkflowRule. I believe calling it Workflow makes more sense

Although we could replace WorkflowRules to Workflow right away. I would prefer we keep both and deprecate WorkflowRules class. Allowing users to migrate to it at their own pace before we remove it in next major version.

  • RuleActions - RuleActions is a collection of Actions that can be applied to a rule. Both OnSuccess and OnFailure are actions defined by ActionInfo class and plural makes more sense to me.

Coming back to EF model for RuleActions, I think it would be better to store it as a mapping of
|Rulename | ActionType | ActionName|

with RuleName and ActionType acting as composite key.

Although it would be a custom model. It should be fine for users trying to configure on their own.

@David-Moreira
Copy link
Contributor

Good point with the workflow entity... I was only thinking about these in the context of pluralization, Workflowrule would be misleading, yea. If its a breaking change might as well do it well right away. 👍

@alexreich
Copy link
Contributor Author

alexreich commented Jul 29, 2021

Lots of simplification - EF now is only 2 tables with Actions column in Rules stored as
{"OnSuccess":{"Name":"SuccessTest","Context":{"foo":123}},"OnFailure":{"Name":"FailTest","Context":{"bar":123}}}.

Made a Data namepsace under RulesEngine with a generic RulesEngineContext which could be inherited for any provider (e.g. RulesEngineDemoContext has SQLite specifics).

Incorporated Ids as non-required fields which acts as identity keys for EF and simplifies other issues.

Put obsolete decoration on WorkflowRules with inheritance to Workflow.

@alexreich
Copy link
Contributor Author

@abbasc52, @David-Moreira Plz let me know if there any other changes needed.

src/RulesEngine/Models/Workflow.cs Outdated Show resolved Hide resolved
src/RulesEngine/Models/Workflow.cs Show resolved Hide resolved
src/RulesEngine/RulesCache.cs Outdated Show resolved Hide resolved
src/RulesEngine/RulesCache.cs Outdated Show resolved Hide resolved
@alexreich alexreich requested a review from abbasc52 August 5, 2021 13:24
@alexreich alexreich requested a review from abbasc52 August 6, 2021 11:55
Copy link
Contributor

@abbasc52 abbasc52 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexreich PR looks good. Thanks for all the hard work!
Just gave some minor comment before we can close this 😄

src/RulesEngine/RulesEngine.cs Outdated Show resolved Hide resolved
src/RulesEngine/RulesEngine.cs Outdated Show resolved Hide resolved
src/RulesEngine/Models/Workflow.cs Outdated Show resolved Hide resolved
@alexreich alexreich requested a review from abbasc52 August 10, 2021 11:34
@abbasc52 abbasc52 merged commit 34f77ed into microsoft:main Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants