search

csharpfritz / InstantAPIs

A library that generates Minimal API endpoints for an Entity Framework context.
MIT License
448 stars 57 forks source link

Enabling multiple data providers #57

Open verbedr opened 2 years ago

verbedr commented 2 years ago

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.

csharpfritz commented 2 years ago

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

verbedr commented 2 years ago

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 commented 1 year ago

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.

csharpfritz commented 1 year ago

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 https://github.com/csharpfritz/InstantAPIs/pull/57#issuecomment-1073310671, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAATF4NQA54A7XKC7SXRIOLVA5X3LANCNFSM5RBPW3ZA . You are receiving this because you commented.Message ID: @.***>

verbedr commented 1 year ago

Hi Jeff, thanks for the response. I do understand the problem, therefore I had broken up my single big commit in little commits. The thing I'm a bit lost with is how to handle those small steps.

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 Dries