Parser attribute #97

Eivlisskiv · 2024-09-25T19:14:11Z

Added ConsoleParserAttribute for Parser Methods to be marked and added to the dictionary before adding commands from the ConsoleMethodAttribute
Added TypeSearch helper struct to organize all parser and command methods found in a specified type.
Modified DebugLogConsole.SearchAssemblyForConsoleMethods to use TypeSearch's results and add all parser methods first, followed by command methods.

Usage Example:

[ConsoleParser(typeof(MyType), "My Type")]
public static bool TryParseMyType(string input, out object output)
{
  ...
}

yasirkula · 2024-10-06T16:13:02Z

Thank you for the PR! I find ConsoleParserAttribute useful (though I find ConsoleCustomTypeParserAttribute easier to understand) but I believe the current implementation is too complex for my taste. Here are my concerns:

I don't use LINQ for performance and GC allocation reasons (Unity's official optimization tips also discourage LINQ)
Right now, all types are stored in a TypeSearch[] array which IMO is unnecessary. For this case, I'd iterate over all types as before and store all ConsoleMethodAttributes occurrences in a List. And for all ConsoleParserAttribute occurrences, I'd evaluate them immediately. After iterating through all types, I'd iterate through the cached ConsoleMethodAttribute occurrences in the list. This implementation doesn't require TypeSearch and although it may have issues when another attribute is introduced, I'd like think of it when that happens. At the moment, there are no plans for additional attributes

If you don't feel like changing the current implementation, I could still implement this the way I want in a future update.

Eivlisskiv · 2024-10-06T17:14:08Z

I'd be happy to make changes, I don't especially like the implementation either. I have a rough idea on how to support more attributes I'd like to try as well and see your thoughts on. I will start with making the changes you're requesting.

…thod.

Eivlisskiv · 2024-10-06T19:06:52Z

Added IConsoleMethodInfo to wrap all types of method info attributes.
TypeSearch is now a static class and simply returns a IEnumerable through GetConsoleMethods
The load order of each ConsoleMethodInfo is used to determine which is loaded first.
At order 0, it is immediately loaded and higher orders are loaded afterwards from 1 to x.

… index.

yasirkula · 2024-10-20T14:47:38Z

Thank you for the changes and sorry for my late responses!

The TryBuildFunction function can be removed entirely because Delegate.CreateDelegate already validates the method's signature (only out keyword isn't validated but I can live with that)
I don't use LINQ in any of my plugins and don't plan to change that
Structs are boxed when casted to an interface (IConsoleMethodInfo) so AFAIK there's no benefit in declaring the new types as structs. In fact, I'm completely against declaring those new types in the first place (as I've explained in my previous reply). While the proposed structure is more powerful, I believe it's less efficient due to additional allocations and the enumerator flow, and just unnecessarily complex for the current scenario. I want the implementation to be as simple and efficient as possible (I like KISS principle)

Eivlisskiv · 2024-10-20T17:29:48Z

* The TryBuildFunction function can be removed entirely because Delegate.CreateDelegate already validates the method's signature (only `out` keyword isn't validated but I can live with that)

I personally prefer very clear error messages, but I suppose since the message contains the method definition example, it should remain clear anyways.

Renamed TryBuildFunction to TryBuildCustomParameterFunction.
Removed method info validation.

* I don't use LINQ in any of my plugins and don't plan to change that

There should not be any linq used, I've removed it all. I noticed I missed a using that I have now removed, but there's not linq methods being used.

Removed unused using System.Linq

* Structs are boxed when casted to an interface (IConsoleMethodInfo) so AFAIK there's no benefit in declaring the new types as structs. In fact, I'm completely against declaring those new types in the first place (as I've explained in my previous reply). While the proposed structure is more powerful, I believe it's less efficient due to additional allocations and the enumerator flow, and just unnecessarily complex for the current scenario. I want the implementation to be as simple and efficient as possible (I like KISS principle)

I wanted your feedback on the new enumerator first, but my intentions where to use the attributes to hold IConsoleMethodInfo instead. I believe doing so would align with your concerns, so I will go along with the changes I intended.

Removed IConsoleMethodInfo and the two structs.
Added ConsoleAttribute abstract class to replace IConsoleMethodInfo
Attributes extend ConsoleAttribute and define their order and load method.

I will be pushing the commits shortly. Some unrelated errors in my project have to be fixed before I can perform tests.

…dles their load order and load method.

yasirkula · 2024-10-26T14:12:41Z

I believe we can store all attributes in a List and then sort that list by Order. No need for TypeSearch and its Dictionary+enumerator approach anymore.

PS. A base class with Order property is good thinking 👍

yasirkula · 2024-10-26T14:17:29Z

Plugins/IngameDebugConsole/Scripts/DebugLogConsole.cs

-				foreach( Type type in assembly.GetExportedTypes() )
+				IEnumerable<ConsoleAttribute> consoleMethods = TypeSearch.GetConsoleMethods(assembly.GetExportedTypes());
+				foreach (ConsoleAttribute consoleMethod in consoleMethods)
 				{
-					foreach( MethodInfo method in type.GetMethods( BindingFlags.Static | BindingFlags.Public | BindingFlags.DeclaredOnly ) )
-					{
-						foreach( object attribute in method.GetCustomAttributes( typeof( ConsoleMethodAttribute ), false ) )
-						{
-							ConsoleMethodAttribute consoleMethod = attribute as ConsoleMethodAttribute;
-							if( consoleMethod != null )
-								AddCommand( consoleMethod.Command, consoleMethod.Description, method, null, consoleMethod.ParameterNames );
-						}
-					}
+					consoleMethod.Load();


So, can you restore this old code and instead of calling AddCommand, just fill the List? Then we can Sort that list and process its elements.

yasirkula · 2024-10-26T14:19:56Z

Plugins/IngameDebugConsole/Scripts/DebugLogConsole.cs

+		public static void AddCustomParameterType(MethodInfo method, Type type, string readableName)
+		{
+			if (TryBuildCustomParameterFunction(method, out ParseFunction func, false))
+				AddCustomParameterType(type, func, readableName);
+		}
+
+		private static bool TryBuildCustomParameterFunction(MethodInfo method, out ParseFunction function, bool validate)
+		{
+			try
+			{
+				function = (ParseFunction)Delegate.CreateDelegate(typeof(ParseFunction), method);
+				return true;
+			}
+			catch (Exception e)
+			{
+				const string format = "Parser Method {0}.{1} is Invalid.\n{2}\nex: public static bool {1}(string input, out object result)";
+				string error = string.Format(format, method.DeclaringType.FullName, method.Name, e.Message);
+				Debug.LogError(error);
+				function = null;
+				return false;
+			}
+		}
+


TryBuildCustomParameterFunction isn't called elsewhere so could you combine these two functions?
PS. The function can be internal so that it can't be called from user code.
PPS. If we remove catch altogether, I believe we won't lose any important information since Delegate.CreateDelegate's exception message is informative by itself. In the worst case, if user can't understand the message, they can double click the exception and clearly see that a ParseFunction signature is expected.

Do we want to break when an exception is throw or just to skip this one method? If we don't catch here, all the next methods will be ignored.

I'm good with an unhandled exception in this case. There is an error that must be resolved and in both approaches, that error will be logged as soon as the developer enters Play mode. An exception will force the developer to resolve the issue which is a win-win in my book: it's simpler and more effective.

yasirkula · 2024-10-26T14:20:36Z

Plugins/IngameDebugConsole/Scripts/DebugLogConsole.cs

@@ -439,7 +455,7 @@ private static void AddCommand( string command, string description, string metho
 			AddCommand( command, description, method, instance, parameterNames );
 		}

-		private static void AddCommand( string command, string description, MethodInfo method, object instance, string[] parameterNames )
+		public static void AddCommand( string command, string description, MethodInfo method, object instance, string[] parameterNames )


How about internal?

For this method and the other mentioned before, making them internal so they can't be called from user code means they won't be able to use these methods if the users make some additional custom Console Attributes.

That shouldn't be a problem since there are already Console Attributes for these functions. If a user creates an alternative to ConsoleMethodAttribute, then I'd very much like to hear their reasoning.

When a user types DebugLogConsole.AddCommand to add a command via Scripting API, I want them to use the documented public functions and not these particular functions.

yasirkula · 2024-10-26T14:23:10Z

Plugins/IngameDebugConsole/Scripts/Attributes/ConsoleCustomTypeParserAttribute.cs

+		public readonly Type type;
+		public readonly string readableName;
+
+		public override int Order => 0;


Personally, I love this syntax but it isn't supported on very old Unity versions and I plan to support them for as long as possible. Could you use old syntax here and at other similar location(s): public override int Order { get { return 0; } }

…AssemblyForConsoleMethods;

yasirkula · 2024-10-26T18:42:53Z

Plugins/IngameDebugConsole/Scripts/DebugLogConsole.cs

-				foreach( Type type in assembly.GetExportedTypes() )
+				Type[] types = assembly.GetExportedTypes();
+				List<ConsoleAttribute> methods = new();
+				const BindingFlags BindingAttr = BindingFlags.Static | BindingFlags.Public | BindingFlags.DeclaredOnly;
+				for (int t = 0; t < types.Length; t++)
 				{
-					foreach( MethodInfo method in type.GetMethods( BindingFlags.Static | BindingFlags.Public | BindingFlags.DeclaredOnly ) )
+					MethodInfo[] typeMethods = types[t].GetMethods(BindingAttr);
+					for (int m = 0; m < typeMethods.Length; m++)
 					{
-						foreach( object attribute in method.GetCustomAttributes( typeof( ConsoleMethodAttribute ), false ) )
+						MethodInfo method = typeMethods[m];
+						IEnumerable<ConsoleAttribute> attributes = method.GetCustomAttributes<ConsoleAttribute>(true);
+						foreach (ConsoleAttribute attribute in attributes)
 						{
-							ConsoleMethodAttribute consoleMethod = attribute as ConsoleMethodAttribute;
-							if( consoleMethod != null )
-								AddCommand( consoleMethod.Command, consoleMethod.Description, method, null, consoleMethod.ParameterNames );
+							attribute.SetMethod(method);
+							methods.Add(attribute);


Thank you for the changes.

Could we make the diff minimalistic by changing only the necessary lines? This will help with Git's Blame/Annotate functionality. Ideally, only the code inside the innermost foreach should change.

I'd advise against casting arrays or lists to IEnumerable because foreach(list) and foreach(array) (these use duck typing) are more optimized than foreach(ienumerable) (you can check out their GC allocations in Profiler).

new() isn't supported on older Unity versions. I also prefer this approach in my personal projects but atm not in my plugins.

* Could we make the diff minimalistic by changing only the necessary lines? This will help with Git's Blame/Annotate functionality. Ideally, only the code inside the innermost foreach should change.

Sure, I personally try to always for loops over foreach loops.

* I'd advise against casting arrays or lists to IEnumerable because `foreach(list)` and `foreach(array)` (these use duck typing) are more optimized than `foreach(ienumerable)` (you can check out their GC allocations in Profiler).

GetCustomAttributes returns a IEnumerable and not a list or array. I'll switch to the non generic one like the original code did.

* new() isn't supported on older Unity versions. I also prefer this approach in my personal projects but atm not in my plugins.

Force of habit, will fix!

…inal code;

…d removed try catch;

…d changes;

yasirkula · 2024-10-27T10:18:26Z

Thank you for the changes.

Eivlisskiv added 3 commits September 24, 2024 19:14

Added ConsoleParser attribute for static parameter parsing methods.

7e938fc

parser validations fixes

b49a015

replace interpolation for string fromatting

7e658aa

converted TypeSearch into a static class with flattened getmethods me…

d72c012

…thod.

renamed IConsoleMethod to IConsoleMethodInfo and fixed the order loop…

5b810f8

… index.

Converted ConsoleMethodInfo to ConsoleAttribute to let attributes han…

8c151e4

…dles their load order and load method.

yasirkula reviewed Oct 26, 2024

View reviewed changes

simplefied method search with sorted list and moved it back to Search…

c1baad7

…AssemblyForConsoleMethods;

yasirkula reviewed Oct 26, 2024

View reviewed changes

Eivlisskiv added 4 commits October 26, 2024 16:14

changed SearchAsseblyForConsoleMethods to more closely match the orig…

d8ec81c

…inal code;

merged TryBuildCustomParameterFunction into AddCustomParameterType an…

e801c2f

…d removed try catch;

internalized the AddCommand method used by ConsoleMethodAttribute;

7119eb5

restored formatting in SearchAssemblyForConsoleMethods to avoid marke…

51ea685

…d changes;

yasirkula merged commit fe646cf into yasirkula:master Oct 27, 2024

yasirkula added a commit that referenced this pull request Oct 27, 2024

Slight simplifications to #97 + Updated README

6578689

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parser attribute #97

Parser attribute #97

Eivlisskiv commented Sep 25, 2024

yasirkula commented Oct 6, 2024

Eivlisskiv commented Oct 6, 2024

Eivlisskiv commented Oct 6, 2024 •

edited

Loading

yasirkula commented Oct 20, 2024

Eivlisskiv commented Oct 20, 2024 •

edited

Loading

yasirkula commented Oct 26, 2024 •

edited

Loading

yasirkula Oct 26, 2024

yasirkula Oct 26, 2024 •

edited

Loading

Eivlisskiv Oct 26, 2024

yasirkula Oct 26, 2024

yasirkula Oct 26, 2024

Eivlisskiv Oct 26, 2024

yasirkula Oct 26, 2024

yasirkula Oct 26, 2024

yasirkula Oct 26, 2024

Eivlisskiv Oct 26, 2024 •

edited

Loading

yasirkula commented Oct 27, 2024

Parser attribute #97

Parser attribute #97

Conversation

Eivlisskiv commented Sep 25, 2024

yasirkula commented Oct 6, 2024

Eivlisskiv commented Oct 6, 2024

Eivlisskiv commented Oct 6, 2024 • edited Loading

yasirkula commented Oct 20, 2024

Eivlisskiv commented Oct 20, 2024 • edited Loading

yasirkula commented Oct 26, 2024 • edited Loading

Choose a reason for hiding this comment

yasirkula Oct 26, 2024 • 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

Eivlisskiv Oct 26, 2024 • edited Loading

Choose a reason for hiding this comment

yasirkula commented Oct 27, 2024

Eivlisskiv commented Oct 6, 2024 •

edited

Loading

Eivlisskiv commented Oct 20, 2024 •

edited

Loading

yasirkula commented Oct 26, 2024 •

edited

Loading

yasirkula Oct 26, 2024 •

edited

Loading

Eivlisskiv Oct 26, 2024 •

edited

Loading