Update merge fix #218
Update merge fix #218
Conversation
|
It is not a good idea to chain PRs for unrelated issues. Since this PR includes code from the previous PRs, one can't easily distinguish which code is relevant to the issue. This makes reviewing the code much harder. I've also made cleanups in #225 after merging #216, so this PR might get merge conflicts. It might be easier for you to close this PR and make a brand new one to fix #217. |
| return descending ? -compare : compare; | ||
| }); | ||
| final fields = (parameters['orderedBy'] ?? []); | ||
| for (var index = 0; index < fields.length; index++) { |
There was a problem hiding this comment.
Is this necessary to fix 217? Can you explain what needed rework?
There was a problem hiding this comment.
This PR is from 3 months ago so my memory isn't exactly clear. It looks like this might be from one of the PR this one was chained onto. Maybe you edited the other PR and this is seen as a change ? I agree my PR process was messy here...
This seems unrelated by the looks of it. I hope it's not a fix from the other PR that was somehow included here and not in the other though.
I'll resubmit this one with only appropriate changes eventually.
There was a problem hiding this comment.
Thank you! And sorry for taking so long to review. It makes us forget the context. That's why I merged #216 as is, since it fixed the issue, then cleaned up the code myself.
There was a problem hiding this comment.
For info I'm really busy right now. This is still on my mind, but I have priorities, I don't think I will be able to come back to this in the immediate future. I'm using my fork for now, as it fixed the issues.
| final movieFound = searchResults.docs.first.data(); | ||
| expect(movieFound.title, equals('Robot from the future')); | ||
| }); | ||
| // test('startAt with converters', () async { |
There was a problem hiding this comment.
Would you mind restoring the tests?
Agreed. I was trying to resolve a specific query issue that affected me personally and applied every fix in succession to fix my problem. This is not exactly a good excuse but here we are. |
fixes #217
should merge #215 #216 prior to this