search

UnoSD / Moq.Dapper

Moq extensions for Dapper methods.
GNU General Public License v2.0
173 stars 78 forks source link

Align DbConnexion and IDbConnexion + add Dapper async methods #40

Closed Requette closed 5 years ago

Requette commented 5 years ago

Add new features : QueryFirstAsync QueryFirstOrDefaultAsync QuerySingleAsync QuerySingleOrDefaultAsync

Refactoring to unify DbConnexion and IDbConnexion management Add multiple unit test

Lonli-Lokli commented 5 years ago

@Tewr is there any chance to review & merge it?

@Requette Looking into new Mocks, will it be easy to add ExecuteScalarAsync & splitOn functionality into library?

eeskildsen commented 5 years ago

@Lonli-Lokli @Tewr @UnoSD Seconding the request to review and merge. This would be great to have.

UnoSD commented 5 years ago

As usual I have not had any time, but I will try and glance it now if it doesn't require too long. But thank you for the pull request.

UnoSD commented 5 years ago

I spent some time looking at it, but there are a lot of non-functional changes (I.E. renaming of variables and moving stuff around etc) I've started rolling some back, but I ended up spending the time I had to remove those and not actually looking at the code. I'll try and get back to it this Saturday.

Before I look further, there is one test that is failing that stops me from merging it and building a new version.

Thanks.

Lonli-Lokli commented 5 years ago

@UnoSD is there any update? Thanks!

UnoSD commented 5 years ago

@Lonli-Lokli I managed to get some time to have a look at it, but unfortunately a test is broken so I can't merge it until it's fixed.

Lonli-Lokli commented 5 years ago

@UnoSD do you need help from @Requette or community to fix it?

UnoSD commented 5 years ago

@Lonli-Lokli any help is appreciated, I haven't touched it in a while so it takes me even longer to figure out how to review PRs.

I put some effort in removing all non-functional changes from here (variable renames, reformatting, moving code), but in the end I realised tests were not passing.

I don't have time to fix it myself; I'll do the best I can to integrate changes from the community, but they need to be consistent with the repo's code style and, obviously, shouldn't break existing code.

Lonli-Lokli commented 5 years ago

@UnoSD I do not have rights for @Requette repository, but failing test can be fixed with this code: Moq.Dapper/DbCommandSetup.cs

before:

            mockResult(commandMock, () => result); 

            mock
                .Setup(m => m.CreateCommand())
                .Returns(commandMock.Object);

            return setupMock.Object;

after: 

            mockResult(commandMock, () => result); 

            mock
                .As<IDbConnection>()
                .Setup(m => m.CreateCommand())
                .Returns(commandMock.Object);

            return setupMock.Object;
Requette commented 5 years ago

Hello,

I will update the PR the weekend.

UnoSD commented 5 years ago

Thanks @Lonli-Lokli @Requette once the tests are passing I'll merge it.

@Lonli-Lokli another person eyeballing the PR would be great if you feel like it. I will likely only glance it and merge it if the tests are passing.

Requette commented 5 years ago

I fix the TU and the property name, even if it isn't a functional change, I think I could only be better.

UnoSD commented 5 years ago

@Requette I agree it needs to be renamed, but I prefer not to have mixed commits. I'll make the change separately.

Requette commented 5 years ago

Ok no problem

Lonli-Lokli commented 5 years ago

@UnoSD as for me this PR is ok, just maybe one of you will update readme with sample usage as well?

Requette commented 5 years ago

I add some new method example (the list isn't exhaustive)

UnoSD commented 5 years ago

Sorry, I tried really hard but I can't merge this.

I would reconsider it if:

1) Changes are kept to the minimum, no "change1" + "change2" PRs 2) Refactoring is fine, but not in the same PR as other changes; classes moved, property renamed, packages updated, metadata changes cannot be in the same PR, I cannot spend my limited time reverting those

I am not using this library anymore therefore I am only doing this for the community in my limited spare time, if I have to take time to merge a PR I would like to see the smallest functional change + it's corresponding test so I don't have to waste days trying to understand what's going on and reverting other changes. 25+ files is not a small PR. When I was a developer I would have rejected it merely on the basis of the amount of file if that wasn't strictly necessary.

I tried to spend an hour today to merge it because it seems like something people would really like to see in the library, but I can't spend more time on this. Please consider removing ALL changes unrelated to the functionality added and split those into the smallest chunks possible in multiple PRs; in that case I would be happy to merge it, but, as it is, I am going to have to close it.

Lonli-Lokli commented 5 years ago

This PR has all functions mentioned in first post, no more and no less. I do not get why multiple goals MUST be in separate PRs.

UnoSD commented 5 years ago

I need to see the changes in isolation so it is as easy and fast as possible for me to merge it, I do not have time to review 20 files and also fix code style and all other random changes in the branch. I tried, but I ended up again spending time revering renamed properties and all the other stuff I mentioned above. I'd like to remind I do this for free so it should, at very least, be facilitated by the others. I appreciate the contributions, but I rather spend my weekends doing something else, honestly.