Removed fallback return type renderer #807

hol430 · 2023-02-17T05:07:39Z

Fixes #806

I agree that my contribution may be licensed either under MIT or any version of LGPL license.

badcel

Thank you for your PR. All in all this is already pretty good 👍

There are a lot of remarks as I want to have the code in a pretty consistent style. Most of them are duplicates either way so you can check one file after the other. Please forgive me my strictness but I think it is for the better longterm.

Regarding your git commits, we have to clean them up a bit to have a clean git history. Before doing something ensure your latest changes are pushed to github so you have a copy of your changes in case something goes wrong with the next steps (or even manually copy the code depending on how securly you can manage git). Please do the following:

Rebase your branch onto current main. For this you will probably need to add a new upstream remote to your repos and rebase it onto gir.core/main (example, remember to use rebase instead of merge)
Currently you have 4 commits. As all your changes are actually one we need to make one commit from your 4. The easiest way is to use some git ui and do a "Soft" reset the the most recent commit from "gir.core/main". This will stash all your changes and the commits will be gone locally. Now create one new commit with a meaningfull message.
If you are happy with your local changes "force push" your branch to github and overwrite your existing branch on github. The PR on gir.core will update automatically afterwards. Now your local repository and the one on github should be identical again and the 4 commits should be gone and be replaced by one.

Edit: please add the following to your pr description:

I agree that my contribution may be licensed either under MIT or any version of LGPL license.

src/Generation/Generator/Renderer/Public/ReturnType/From/RenderableScalarReturnType.cs

src/Generation/Generator/Renderer/Public/ReturnType/From/IRenderableReturnType.cs

badcel · 2023-03-01T19:40:31Z

src/Generation/Generator/Renderer/Public/ReturnType/From/IRenderableReturnType.cs

@@ -0,0 +1,7 @@
+namespace Generator.Renderer.Public;
+
+internal interface IRenderableReturnType


Please do not prefix the interface with an "I". A better name would be ReturnTypeConverter.

Please move the interface into the namespace: Generator.Renderer.Public.ReturnType;

Please move the interface one level up out of the "From" / "Converter" folder.

Please rename the "From" folder to "Converter".

Regarding not prefixing an interface with "I". I know it's not idiomatic but an interface should not be more specific than an implementation thus adding a prefix does not make sense for me. An interface defines which methods must be present. A class implementing this interface adds some explicit code, thus it has to be more specific naming wise than it's interface.

badcel · 2023-03-01T19:46:24Z

src/Generation/Generator/Renderer/Public/ReturnType/RenderableReturnTypeFactory.cs

+        new StringArrayReturnType(),
+
+        // fallback renderer
+        // new StandardReturnType()


Please remove comments as they do not serve a purpose if the code is not in use. Either it is used or it is obsolete and can be removed.

// fallback renderer // new StandardReturnType()

src/Generation/Generator/Renderer/Public/ReturnType/RenderableReturnTypeFactory.cs

src/Generation/Generator/Renderer/Public/ReturnType/From/RecordArrayType.cs

src/Generation/Generator/Renderer/Public/ReturnType/From/VoidReturnType.cs

src/Generation/Generator/Renderer/Public/ReturnType/From/RecordArrayType.cs

src/Generation/Generator/Renderer/Public/ReturnType/From/StringArrayReturnType.cs

src/Generation/Generator/Renderer/Public/ReturnType/From/VoidReturnType.cs

badcel

All in all this is nearly done, good work!

One class needs to be moved into another namespace. The rest are only stylistic changes.

Please rebase on current main and keep only one commit.

badcel · 2023-03-08T09:00:47Z

src/Generation/Generator/Renderer/Public/ReturnType/ReturnTypeRenderer.cs

@@ -1,6 +1,6 @@
-namespace Generator.Renderer.Public;
+namespace Generator.Renderer.Public.ReturnType;


Please do not change the namespace here. As this is the "public render class" (meaning it creates a string from a GirModel.ReturnType) it should not be hidden in some namespace. All the other classes in this folder are in the RetunType namespace to hide them away as they are an implementation detail nobody should care about them except the ReturnTypeRenderer.

badcel · 2023-03-08T09:01:52Z

src/Generation/Generator/Renderer/Public/Class/ClassConstructors.cs

@@ -36,7 +36,7 @@ private static string Render(GirModel.Constructor constructor, GirModel.Class cl
        {
            var parameters = ParameterToNativeExpression.Initialize(constructor.Parameters);

-            var newKeyWord = Class.HidesConstructor(cls.Parent, constructor)
+            var newKeyWord = Model.Class.HidesConstructor(cls.Parent, constructor)


Please keep the code as is. The Model. namespace is obsolete.

badcel · 2023-03-08T09:03:40Z

...ration/Generator/Renderer/Public/InterfaceImplementation/InterfaceImplementationFramework.cs

@@ -19,9 +19,9 @@ namespace {Namespace.GetPublicName(@interface.Namespace)};
 // AUTOGENERATED FILE - DO NOT MODIFY

 {PlatformSupportAttribute.Render(@interface as GirModel.PlatformDependent)}
-public sealed partial class {Interface.GetImplementationName(@interface)} : GObject.Object, {@interface.Name}
+public sealed partial class {Model.Interface.GetImplementationName(@interface)} : GObject.Object, {@interface.Name}


Please keep this file as is. The additional Model. namespacing information is obsolete.

badcel · 2023-03-08T09:04:37Z

...ration/Generator/Renderer/Public/InterfaceImplementation/InterfaceImplementationFunctions.cs

@@ -20,7 +20,7 @@ namespace {Namespace.GetPublicName(@interface.Namespace)};

 // AUTOGENERATED FILE - DO NOT MODIFY

-public partial class {Interface.GetImplementationName(@interface)} : GObject.GTypeProvider
+public partial class {Model.Interface.GetImplementationName(@interface)} : GObject.GTypeProvider


Please keep the file as is. The Model. namespace information is obsolete.

badcel · 2023-03-08T09:05:11Z

...neration/Generator/Renderer/Public/InterfaceImplementation/InterfaceImplementationMethods.cs

@@ -20,7 +20,7 @@ namespace {Namespace.GetPublicName(@interface.Namespace)};

 // AUTOGENERATED FILE - DO NOT MODIFY

-public partial class {Interface.GetImplementationName(@interface)}
+public partial class {Model.Interface.GetImplementationName(@interface)}


Please keep the file as is.

badcel · 2023-03-08T09:05:31Z

...ation/Generator/Renderer/Public/InterfaceImplementation/InterfaceImplementationProperties.cs

@@ -20,7 +20,7 @@ namespace {Namespace.GetPublicName(@interface.Namespace)};

 // AUTOGENERATED FILE - DO NOT MODIFY

-public partial class {Interface.GetImplementationName(@interface)}
+public partial class {Model.Interface.GetImplementationName(@interface)}


Please keep the file as is.

badcel · 2023-03-08T09:06:46Z

src/Generation/Generator/Renderer/Public/RecordClass.cs

@@ -6,8 +6,8 @@ internal static class RecordClass
 {
    public static string Render(GirModel.Record record)
    {
-        var name = Record.GetPublicClassName(record);
-        var internalHandleName = Record.GetInternalHandleName(record);
+        var name = Model.Record.GetPublicClassName(record);


Please keep the file as is (4 obsolete instance of Model.)

badcel · 2023-03-08T09:09:39Z

src/Generation/Generator/Renderer/Public/ReturnTypeToManagedExpression/Converter/Record.cs

@@ -11,7 +11,7 @@ public bool Supports(AnyType type)
    public string GetString(GirModel.ReturnType returnType, string fromVariableName)
    {
        if (returnType.IsPointer)
-            return $"new {ReturnType.Render(returnType)}({fromVariableName})";
+            return $"new {Renderer.Public.ReturnType.ReturnTypeRenderer.Render(returnType)}({fromVariableName})";


Please do not add additional namespacing information:

This is sufficient:
return $"new {ReturnTypeRenderer.Render(returnType)}({fromVariableName})";

The fallback return type renderer has been removed, so that method rendering in interfaces is more consistent with the rendering process in classes. Previously, the fallback return type renderer would allow some methods to be rendered in an interface but not in implementations of that interface, which resulted in broken bindings.

badcel

Looks good, thank you for your contribution.

hol430 force-pushed the bug/InterfaceRenderer branch from 15e743f to 29e1bcb Compare February 28, 2023 05:21

hol430 changed the title ~~Don't render methods in interfaces if they can't be implemented~~ Removed fallback return type renderer Feb 28, 2023

badcel requested changes Mar 1, 2023

View reviewed changes

hol430 force-pushed the bug/InterfaceRenderer branch from d0376fb to b78b970 Compare March 2, 2023 01:39

badcel requested changes Mar 11, 2023

View reviewed changes

hol430 force-pushed the bug/InterfaceRenderer branch from b78b970 to ba2f91a Compare March 15, 2023 23:28

badcel approved these changes Mar 16, 2023

View reviewed changes

badcel merged commit 13e4e8b into gircore:main Mar 16, 2023

hol430 deleted the bug/InterfaceRenderer branch March 19, 2023 22:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed fallback return type renderer #807

Removed fallback return type renderer #807

hol430 commented Feb 17, 2023 •

edited

Loading

badcel left a comment •

edited

Loading

badcel Mar 1, 2023

badcel Mar 1, 2023

badcel left a comment •

edited

Loading

badcel Mar 8, 2023

badcel Mar 8, 2023

badcel Mar 8, 2023

badcel Mar 8, 2023

badcel Mar 8, 2023

badcel Mar 8, 2023

badcel Mar 8, 2023

badcel Mar 8, 2023

badcel left a comment

		@@ -0,0 +1,7 @@
		namespace Generator.Renderer.Public;

		internal interface IRenderableReturnType

		@@ -1,6 +1,6 @@
		namespace Generator.Renderer.Public;
		namespace Generator.Renderer.Public.ReturnType;

Removed fallback return type renderer #807

Removed fallback return type renderer #807

Conversation

hol430 commented Feb 17, 2023 • edited Loading

badcel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

badcel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

badcel left a comment

Choose a reason for hiding this comment

hol430 commented Feb 17, 2023 •

edited

Loading

badcel left a comment •

edited

Loading

badcel left a comment •

edited

Loading