-
Notifications
You must be signed in to change notification settings - Fork 1
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
Adds entity CreateAsync prototype on Repo/AppController #295
Conversation
Codecov Report
@@ Coverage Diff @@
## master #295 +/- ##
=========================================
- Coverage 89.81% 89.22% -0.6%
=========================================
Files 104 104
Lines 2868 2877 +9
Branches 299 299
=========================================
- Hits 2576 2567 -9
- Misses 198 215 +17
- Partials 94 95 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me that is not a acceptable solution, because the 2 CreateAsync methods are too similar but work with very different inputs (candidates vs entities).
At least it should be renamed CreateEntitiesFromCandidatesAsync and CreateEntitiesAsync for instance, to avoid confusion.
But, as we said some weeks ago, what we should really do is get rid of Candidates on the Domain layer, and trash the actual CreateAsync method, and replace it with your new method.
What you could do, now that we follow semver, is :
- put this code into a v3.0.1 or v3.1 since there is no breaking change
- make an issue for the v4.0 with my below introduction, in order not to forget to refactor all that stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming things... 😄
@rducom I would name everything |
Nico your points are adressed, I merge this one
Resolve #252