Enabling multiple data providers #57
Conversation
verbedr
force-pushed
the
feature/multi-repo
branch
2 times, most recently
from
2c0d002 to
81d72b3
Compare
|
This is a huge change, removes a bunch of public-facing objects which will break compatibility. We would prefer a more focused pull-request that delivers just one feature from the issues list. Some of these changes may be valuable, but its too many items changing all at once |
|
No problem, if there is something I can change, so it is more tangible, let me know. It was a sort of concept or set of ideas to solve the multiple repository problem. Perhaps there is something salvable. |
verbedr
force-pushed
the
feature/multi-repo
branch
2 times, most recently
from
5e4cac5 to
ffb78e0
Compare
Using a central test fixture to setup the tests around the configuraiton builder so we do not duplicate the creation of the setup mock.
The instiant apis' config class only holds one property. Instead of using a class to hold one property as a dto, pass only that property. Applying the yagni (yet) principle.
Renaming the InstantAPIsConfigBuilder to the InstantAPIsBuilder as there is no need to make this distinction and the builder represent beter of what it is doing. Also renamed the file to match the class
The build produced a couple warnings around null references and had some bad project references. In order not to hide real warnings that need to resolve. The idea is to make sure there are zero warnings
To be more inline with other library setups we rename the ServiceOptions to plain Options. There is no specific need for ServiceOptions in a plugin.
- Renaming existing generic types to convention Txxx - Add generic types for set, entity, key - Use an expression to identity the dbcontext.set. This ensure better/correct configuration
- added an interface with factory - included ef core provider - included json provider - update the test programs to utilize the repository approach
- implemented an update - insert should take max of the key + 1, instead of just counting - remove unused constructor prarameter
verbedr
force-pushed
the
feature/multi-repo
branch
from
ffb78e0 to
625abfc
Compare
|
So I tried to break up the single commit in different steps to make it more understandable and rebased it from the new main branch. If there is anything else I can do to make it more acceptable, let me know. |
|
The volume of the changes, and to things that weren't part of the
requirements are the problem I have. Without documentation that shows how
to use your new configuration and objects, its confusing.
Once example: The configuration system was deleted and completely
rebuilt. Was that needed? Not sure... but the other contributors to the
project don't get a chance to review and discuss that change because its
part of this larger change.
Why were the EF interactions completely rewritten? It looks like you're
trying to simplify the interaction, but I can't tell what the value is of
this change without reading the entire pull-request and getting wrapped up
in the other changes
Can we focus on changing one thing at a time so that we can all understand
how this fits together?
Jeff
…On Sun, Mar 20, 2022 at 2:53 PM Dries ***@***.***> wrote:
No problem, if there is something I can change, so it is more tangible,
let me know. It was a sort of concept or set of ideas to solve the multiple
repository problem. Perhaps there is something salvable.
—
Reply to this email directly, view it on GitHub
<#57 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAATF4NQA54A7XKC7SXRIOLVA5X3LANCNFSM5RBPW3ZA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Hi Jeff, For example, the first commit is using a single fixture to set up and teardown similar unit tests. As it isn't a real issue for the application, I'm reluctant to create an Issue for this. But, should I instead create a single PR with only the first commit and start up a discussion with those changes? I can do this step by step for every single commit if this would make it more open and allows others to comment or even pitch in different ideas. Let me know if that would be workable for you. Kr |
This is a POC/idea to implement the #4 issue. I know this is a big code change at once, my apologies for this. If some changes conflicts with the initial idea or concept of this project, again my apologies.
I hope this can kickstart a discussion on how to implement it.
PS there are a lot of parts in the code that can be improved or made more robust.