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

module support #7

Draft
wants to merge 37 commits into
base: main
Choose a base branch
from
Draft

module support #7

wants to merge 37 commits into from

Conversation

wingyplus
Copy link
Owner

@wingyplus wingyplus commented Jun 4, 2024

Tasks

  • Every library in the SDK must be netstandard 2.0!
  • Generate necessary attributes and interfaces for user code.
  • Generate registration method to convert user code into Dagger.Module for registering to the Dagger Engine.
    • We need to traverse from root class (Potato.Potato for example) and generate child type from their.
  • Generate invoke function.
  • Make Dagger.SDK support JSON serialization
    • We need it for returns the value when function get call.
    • For example, Dagger.Container must serialize into Dagger.ContainerID.Value and deserialize it back.
    • We can done this in Dagger.SDK.SourceGenerator by implementing json converter.
  • Remove my potato. 🥔

Idea on the next PR

Closes #3

@wingyplus wingyplus marked this pull request as draft June 4, 2024 17:33
@@ -320,7 +320,7 @@ private static string RenderArgumentValue(InputValue arg, bool addVarSuffix = fa
}
}

throw new Exception($"The type {arg.Type.OfType.Kind} should not be enter here.");
throw new Exception($"The argument {arg} should not be enter here.");
Copy link
Contributor

Choose a reason for hiding this comment

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

should not be entered

main.go Outdated Show resolved Hide resolved
@pjmagee
Copy link
Contributor

pjmagee commented Jun 4, 2024

This is looking exciting 👍

@wingyplus
Copy link
Owner Author

UPDATE: I just getting register module and simple invoke works in potato example. I also hit a bug that the SDK cannot retrieve array result, cause we cannot inject input arguments into our method correctly.

{
var potato = JsonSerializer.Deserialize<Potato.Potato>(ParentJson.Value);
Debug.Assert(potato != null, nameof(potato) + " != null");
return await potato.Echo("C#");
Copy link
Owner Author

Choose a reason for hiding this comment

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

I just hit a bug that program got crash when asking input args. So I do temporary hard code argument here to make it works.

@@ -0,0 +1,13 @@
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Owner Author

Choose a reason for hiding this comment

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

If the source generator plan is going well, we don't need this package anymore. 🎉

namespace Dagger.SDK.Mod.SourceGenerator.Tests;

[TestClass]
public class SourceGeneratorTests : VerifyBase
Copy link
Owner Author

Choose a reason for hiding this comment

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

I have discussion with Microsoft team in C# discord. They're recommended me to use https://www.nuget.org/packages/Microsoft.CodeAnalysis.CSharp.SourceGenerators.Testing#supportedframeworks-body-tab rather than Verify. I don't see much example from it but it would be nice if we strict with the Microsoft tools.

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines 27 to 28
postInitializationContext.AddSource("Dagger.SDK.Mod_Attributes.g.cs", ModuleAttributesSource());
postInitializationContext.AddSource("Dagger.SDK.Mod_Interfaces.g.cs", ModuleInterfacesSource());
Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm current stuck with the unit tests because in unit test, it has no reference to the Dagger.SDK.Mod, cause it to fail because it cannot find any class related to our SDK. That's why I put it to generator.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think its difficult to convert to source generator once starting with reflection. It makes you need to change how to design/solution with TDD from the beginning

Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Dagger.SDK.Mod hold the logic for construct and invoke function. Current
implmentation using `System.Reflection` to scan class and method.

Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
And fix bug return list type broken.

Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Introduce a library to uses for generate sources via Source Generator.

Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
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.

Basic Module support
2 participants