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

Add ExpressionMethod Generator #2695

Closed
wants to merge 21 commits into from
Closed

Conversation

viceroypenguin
Copy link
Contributor

@viceroypenguin viceroypenguin commented Dec 15, 2020

Fixes #2687

  • Implement Generators framework
  • Implement ExpressionMethodGenerator
  • Implement Diagnostic messages for error messages to users.
  • Implement SyntaxReceiver

@viceroypenguin viceroypenguin added this to the 3.3.0 milestone Dec 15, 2020
@viceroypenguin viceroypenguin self-assigned this Dec 15, 2020
@viceroypenguin viceroypenguin marked this pull request as ready for review December 20, 2020 03:36
Copy link
Contributor

@MaceWindu MaceWindu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
Does it make sense to handle some C# expressions, not supported by S.L.E, like ??, ?.?

NuGet/linq2db.nuspec Outdated Show resolved Hide resolved
Source/LinqToDB.Generators/AnalyzerReleases.Unshipped.md Outdated Show resolved Hide resolved
Source/LinqToDB.Generators/LinqToDB.Generators.csproj Outdated Show resolved Hide resolved
@viceroypenguin
Copy link
Contributor Author

Does it make sense to handle some C# expressions, not supported by S.L.E, like ??, ?.?

I think the compiler will catch that when actually compiling the generated code. Probably won't be descriptive to the user though. That's a whole 'nother level of analysis though. I'd say let's do that in v2 of this thing if it looks like an issue.

@sdanyliv
Copy link
Member

I'ld like to see more complex tests with several parameters.

@viceroypenguin
Copy link
Contributor Author

I'd like to see more complex tests with several parameters.

Added one more complicated one. Anything specific you'd like to see in another one?

Copy link
Member

@sdanyliv sdanyliv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it looks complicated for end user.

  • Class should be defined as partial
  • Method should be partial
  • No way do define over property.

i suggest to change ExpressionMethodAttribute a little and maybe create base class. It should have at least one virtual method GetLambdaExpression. Then we can derive ExpressionMethodGenerator from base class return lambda in any case.

For generation we can always create static class MySuperClass_generated and generate methods in that class.

@viceroypenguin
Copy link
Contributor Author

  • static classes cannot have inheritance, so using a base class would eliminate automatic generation for extension methods.
  • Also, inheritance reduces options on non-static classes (static method on a DTO that may be tiered for example).
  • Without inheritance, if we want to connect auto-generation to original method, we will have to do convention based reflection for the related class/method. This is not very difficult, since the [GenerateExpressionMethod] attribute will indicate we need to go look for it. However, it would require some minor work to tell l2db how to look.

@viceroypenguin viceroypenguin marked this pull request as draft December 30, 2020 14:01
* Nested Classes
* Properties
* Custom Names
@viceroypenguin
Copy link
Contributor Author

Merry Christmas @sdanyliv - Now doesn't require partial on methods (still requires partial on classes though, no way around that) and supports properties. Still want to write some more unit tests, so not ready for merge yet.

@MaceWindu
Copy link
Contributor

There are 4 oracle tests that changed baselines due to sequence value change.

addressed here #2758

@viceroypenguin
Copy link
Contributor Author

@sdanyliv @MaceWindu any reason for or against this in 3.3.0? I don't particularly care, it was built on an idea from @sdanyliv

@MaceWindu
Copy link
Contributor

/azp run test-all

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MaceWindu
Copy link
Contributor

@viceroypenguin I would say let's merge it after tests passed.

Copy link
Member

@sdanyliv sdanyliv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check my comments.

@viceroypenguin
Copy link
Contributor Author

/azp run test-all

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MaceWindu
Copy link
Contributor

/azp run test-all

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MaceWindu
Copy link
Contributor

I don't understand. Nothing big changed with last commits except merge with master, but now it doesn't build with a lot of errors...

@MaceWindu
Copy link
Contributor

nevermind, just some glitch, after rebuild it complains about code referencing removed MethodName property

@MaceWindu
Copy link
Contributor

@viceroypenguin, reminder about broken branch

@viceroypenguin viceroypenguin modified the milestones: 3.3.0, 3.4.0 Feb 28, 2021
@viceroypenguin
Copy link
Contributor Author

Thanks @MaceWindu. Will revisit when I have time to do so.

@MaceWindu MaceWindu marked this pull request as draft April 17, 2021 10:45
@MaceWindu MaceWindu modified the milestones: 3.4.0, 3.X May 8, 2021
@MaceWindu MaceWindu modified the milestones: 3.X, Future May 22, 2021
@viceroypenguin viceroypenguin deleted the features/generators branch February 26, 2022 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[Idea] Source Generators
6 participants