perpetualKid / GetText.NET

A .NET Standard cross-platform implementation of GNU Gettext
Other
34 stars 11 forks source link

ParserBase: check argument count #37

Closed jorrit closed 2 years ago

jorrit commented 2 years ago

My code contains several other methods named GetString. These methods take no arguments. This causes the extractor to fail with:

Unhandled exception: System.ArgumentOutOfRangeException: Index was out of range. Must be non-negative and less than the size of the collection. (Parameter 'index')
   at GetText.Extractor.Engine.ParserBase`1.GetStrings(SyntaxTree tree) in /_/src/GetText.Extractor/Engine/ParserBase.cs:line 55
   at GetText.Extractor.Engine.SyntaxTreeParser.<Parse>b__1_1(SyntaxTree tree) in /_/src/GetText.Extractor/Engine/SyntaxTreeParser.cs:line 37
   at System.Threading.Tasks.Dataflow.ActionBlock`1.ProcessMessage(Action`1 action, KeyValuePair`2 messageWithId)
   at System.Threading.Tasks.Dataflow.ActionBlock`1.<>c__DisplayClass6_0.<.ctor>b__0(KeyValuePair`2 messageWithId)
   at System.Threading.Tasks.Dataflow.Internal.TargetCore`1.ProcessMessagesLoopCore()
--- End of stack trace from previous location where exception was thrown ---
   at GetText.Extractor.Engine.SyntaxTreeParser.Parse() in /_/src/GetText.Extractor/Engine/SyntaxTreeParser.cs:line 44
   at GetText.Extractor.Program.Execute(FileInfo source, FileInfo target, Boolean unixStyle, Boolean sortOutput, Boolean verbose) in /_/src/GetText.Extractor/Program.cs:line 50
   at GetText.Extractor.Program.<>c.<<Main>b__1_0>d.MoveNext() in /_/src/GetText.Extractor/Program.cs:line 36
--- End of stack trace from previous location where exception was thrown ---
   at System.CommandLine.Invocation.AnonymousCommandHandler.InvokeAsync(InvocationContext context)
   at System.CommandLine.Invocation.InvocationPipeline.<>c__DisplayClass4_0.<<BuildInvocationChain>b__0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass18_0.<<UseParseErrorReporting>b__0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass13_0.<<UseHelp>b__0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass22_0.<<UseVersionOption>b__0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass20_0.<<UseTypoCorrections>b__0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c.<<UseSuggestDirective>b__19_0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass17_0.<<UseParseDirective>b__0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c.<<RegisterWithDotnetSuggest>b__6_0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass9_0.<<UseExceptionHandler>b__0>d.MoveNext()

Ideally, the parser would only consider method invocations on ICatalog instances. However, this seems difficult to achieve. That is why this PR prevents the exception by checking for the number of arguments.

perpetualKid commented 2 years ago

indeed a nice enhancement. while checking for ICatalog instances seems like an ideal solution to work around, this would cause issues with other version of GetText and the like, who all may be using "GetString*" invocations but in different namespaces and so on, so unless all implementions would agree on a common ICatalog base interface, purely using method names as baseline is a reasonable approch, but some additional checks like yours are a sensible way to validate.

jorrit commented 2 years ago

Thanks for merging @perpetualKid ! Could you make a release, if you have time?