Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up query translator/expression namespace and file placement #33782

Merged
merged 1 commit into from
May 24, 2024

Conversation

roji
Copy link
Member

@roji roji commented May 22, 2024

#33755 moved query translator into their own folder, and with the namespace was e.g. Microsoft.EntityFrameworkCore.Cosmos.Query.Internal.Translators; but our analyzer identifies internal namespaces as ending with Internal. Since the main idea here was file placement, I just changed the namespace back to Microsoft.EntityFrameworkCore.Cosmos.Query.Internal (sorry for the churn).

Also did the same with all expression inside provider projects - they're now grouped under their own directories, but keeping the general Microsoft.EntityFrameworkCore.Cosmos.Query.Internal namespace.

Relational is unfortunately a bit of a mess; some expressions are already grouped under an SqlExpressions directory - although many expressions there don't extend SqlExpression (e.g. SelectExpression). Some other expressions are at the query root instead :( Changing namespaces here would be a breaking change; we can still move the files instead just for cleanliness but keep the namespace (or maybe there's some classification system here I didn't notice, @maumar).

@roji roji requested a review from maumar May 22, 2024 18:54
@maumar
Copy link
Contributor

maumar commented May 23, 2024

I think the idea behind putting some expressions outside SqlExpressions directory is that those should be "shaper" expressions, rather than sql query (CollectionResultExpression, StructuralTypeProjectionExpression, JsonQueryExpression) - but note how JsonScalarExpression shows up in sql, so it's in the directory.

I do agree that TableExpressionBase -derived expressions in a bit weird. They do describe SQL query though, so maybe that's why.

@roji
Copy link
Member Author

roji commented May 24, 2024

I think the idea behind putting some expressions outside SqlExpressions directory is that those should be "shaper" expressions, rather than sql query

Yeah, that looks like it - though I think we're not totally consistent... In any case, we can think about cleanup at some point...

@roji roji merged commit ee36df7 into dotnet:main May 24, 2024
7 checks passed
@roji roji deleted the MoveStuffAround branch May 24, 2024 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants