Added collection param to IQuery.For and IQuery.ForIndex method#389
Open
jersiovic wants to merge 1 commit intosebastienros:mainfrom
Open
Added collection param to IQuery.For and IQuery.ForIndex method#389jersiovic wants to merge 1 commit intosebastienros:mainfrom
jersiovic wants to merge 1 commit intosebastienros:mainfrom
Conversation
Author
|
Hi @sebastienros any feedback related to this PR ? |
jersiovic
force-pushed
the
SetCollectionAtForAndForIndex
branch
2 times, most recently
from
311bfab to
02ab692
Compare
sebastienros
reviewed
Nov 8, 2021
| public string _bindingName = "a1"; | ||
| public Dictionary<string, List<Type>> _bindings = new Dictionary<string, List<Type>>(); | ||
| public readonly string _documentTable; | ||
| public string _documentTable; |
Owner
There was a problem hiding this comment.
Maybe remove it from there and only compute it when the query is executed?
sebastienros
reviewed
Dec 3, 2021
|
|
||
| Assert.Equal(2, await session.Query("Col1").For<Person>().With<PersonByNameCol>().Where(x => x.Name.IsInAny<PersonByBothNamesCol>(y => y.Firstname)).CountAsync()); | ||
| Assert.Equal(2, await session.Query().For<Person>(collection:"Col1").With<PersonByNameCol>().Where(x => x.Name.IsInAny<PersonByBothNamesCol>(y => y.Firstname)).CountAsync()); | ||
| Assert.Equal(2, await session.Query("Col2").For<Person>(collection: "Col1").With<PersonByNameCol>().Where(x => x.Name.IsInAny<PersonByBothNamesCol>(y => y.Firstname)).CountAsync()); |
Owner
There was a problem hiding this comment.
Actually I am confused now. Query already has a constructor with a collection, so the For call should use this one. So I read the issue again and I see that you are trying to hack the state of the query. I'd prefer a solution where you can plug a custom "CollectionResolver" that could be configured instead. So the Query() constructor would either take a collection name, or a CollectionResolver, or nothing to use default Collectionresolver from the configuration. That might be even better for your design.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR tries to fix problem extending YesSql functionality at custom Orchard Core module described at this comment #38 (comment)
After the change in this PR a third party can provide ISession, IQuery and ISchemaBuilder implementations which automatically choose the collection based on the type of the object to store/load instead of force the consumer to decide the collection.
In Orchard use an alternate ISession, IQuery and ISchemaBuilder like that allows to change the collection to store/load any type defined in own modules or in modules of the orchard framework