Skip to content

Commit 0e09cac

Browse files
authored
Updated to support better error reporting (#303)
* Updates samples to use distinct namespace and correct descriptions * Updated Kaleidoscope parser return type to `IAstNode?` to match interface and intent. * Changed constructor to `AppControlledDefaultsRootCommand` to make the settings a non-nullable type. * It makes no sense to use that type unless the settings are provided. The whole point is to make it explicit. So if the default behavior is intended then use the parent type OR explicitly provide a default constructed settings. * Changed name of `IRootCommand` to `ICommandLineOptions` to better reflect intent and further isolate from `System.CommandLine` parsing. * Updated the `ArgsParsing.TryParse<>` so that it uses the new `ShowHelpOnErrors` and `ShowTypoCorrections` * Fixed `ReportingTextWriter` adapter so it correctly handles new lines and blank lines * Added nullability checks to `ParseErrorReporterExtensions.CheckAndReportParseErrors` * Added `ParserExtensions` to provide extension that allows parsing from a file (as opposed to parsing a string as input) * Added override of default color mapping for `ColoredConsoleReporter` updated the default colors * Corrected bug in reporting string messages for `ConsoleReporter` * Disabled RUDE OPT-OUT breaking change that forces appending BuildMeta that is NOT desired
1 parent 51669bd commit 0e09cac

File tree

19 files changed

+228
-96
lines changed

19 files changed

+228
-96
lines changed

Directory.Build.props

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,9 @@
6464
<ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
6565
<EnableNETAnalyzers>true</EnableNETAnalyzers>
6666
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
67+
<!-- Disable RUDE OPT-OUT breaking change for entire repo -->
68+
<IncludeSourceRevisionInInformationalVersion>false</IncludeSourceRevisionInInformationalVersion>
69+
6770
<!--
6871
DO NOT ALLOW Implicit using msbuild feature - EVIL; causes mass issues and frustration moving code between projects when one doesn't
6972
support it or uses a different version. Not to mention hiding the actual namespaces involved, making resolving conflicts HARDER.

src/Samples/Kaleidoscope/Chapter8/Options.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33

44
using System.IO;
55

6-
namespace Kaleidoscope
6+
namespace Kaleidoscope.Chapter8
77
{
88
/// <summary>Command line options for this application</summary>
99
internal partial class Options

src/Samples/Kaleidoscope/Chapter8/Options.g..cs

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,25 +9,38 @@
99

1010
using Ubiquity.NET.CommandLine;
1111

12-
namespace Kaleidoscope
12+
namespace Kaleidoscope.Chapter8
1313
{
1414
internal partial class Options
15-
: IRootCommand<Options>
15+
: ICommandLineOptions<Options>
1616
{
1717
public static Options Bind( ParseResult parseResult )
1818
{
1919
return new()
2020
{
2121
// NOTE: an Argument descriptor does NOT have a "Required" property
22+
// binding to a required value that is NOT present AND has no Default value provider
23+
// will generate an exception. There is no "try" semantics. One could argue that
24+
// anything with a default value provider isn't really required but it depends on perspective.
25+
// Any value that MUST be provided by the user (no default is possible) vs. any value that must
26+
// be used by the consumer (default possible if not provided) are different scenarios and each
27+
// can consider the value as "required" as things can't work without it. (That is, evaluation of
28+
// the term "required" must include a subject [that is, an answer to the "to whom/what?"].
29+
// "required" is a concept that is a dependent term and evaluation without a subject is not possible.)
30+
//
31+
// Source generator should make this CRYSTAL CLEAR.
2232
SourcePath = parseResult.GetRequiredValue( Descriptors.SourceFilePath )
2333
};
2434
}
2535

26-
public static AppControlledDefaultsRootCommand BuildRootCommand( CmdLineSettings? settings )
36+
public static AppControlledDefaultsRootCommand BuildRootCommand( CmdLineSettings settings )
2737
{
28-
return new AppControlledDefaultsRootCommand( "Kaleidoscope sample app", settings )
38+
// description is hard coded and should come from generation attribute on the "Options" type
39+
return new AppControlledDefaultsRootCommand( settings, "Kaleidoscope sample app (Chapter 8)" )
2940
{
30-
Descriptors.SourceFilePath
41+
Descriptors.SourceFilePath,
42+
43+
// Additional Options, Arguments, Sub-commands, and directives go here...
3144
};
3245
}
3346
}
@@ -38,8 +51,7 @@ file static class Descriptors
3851
internal static readonly Argument<FileInfo> SourceFilePath
3952
= new Argument<FileInfo>("SourceFilePath")
4053
{
41-
Description = "Path of the input source file [Used as ref for debug information]",
42-
HelpName = "Source file path",
54+
Description = "Path of the input source file to compile",
4355
}.AcceptExistingFileOnly();
4456
}
4557
}

src/Samples/Kaleidoscope/Chapter8/Program.cs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,10 @@
1515

1616
using static Ubiquity.NET.Llvm.Library;
1717

18+
// error CA1506: 'Main' is coupled with '41' different types from '15' different namespaces...
19+
// Total BS. Maybe at the IL level, but hardly true at the source level
20+
#pragma warning disable CA1506
21+
1822
namespace Kaleidoscope.Chapter8
1923
{
2024
public static class Program
@@ -44,7 +48,6 @@ public static int Main( string[] args )
4448
string irFilePath = Path.ChangeExtension( sourceFilePath, ".ll" );
4549
string asmPath = Path.ChangeExtension( sourceFilePath, ".s" );
4650

47-
using var rdr = File.OpenText( sourceFilePath );
4851
using var libLLVM = InitializeLLVM( );
4952
libLLVM.RegisterTarget( CodeGenTarget.Native );
5053

@@ -61,7 +64,7 @@ public static int Main( string[] args )
6164

6265
// time the parse and code generation
6366
var timer = System.Diagnostics.Stopwatch.StartNew( );
64-
var ast = parser.Parse( rdr );
67+
var ast = parser.ParseFrom( sourceFilePath );
6568
if(!errorLogger.CheckAndReportParseErrors( ast ))
6669
{
6770
Module? module = generator.Generate( ast );

src/Samples/Kaleidoscope/Chapter9/Options.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33

44
using System.IO;
55

6-
namespace Kaleidoscope
6+
namespace Kaleidoscope.Chapter9
77
{
88
/// <summary>Command line options for this application</summary>
99
internal partial class Options

src/Samples/Kaleidoscope/Chapter9/Options.g..cs

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,25 +9,38 @@
99

1010
using Ubiquity.NET.CommandLine;
1111

12-
namespace Kaleidoscope
12+
namespace Kaleidoscope.Chapter9
1313
{
1414
internal partial class Options
15-
: IRootCommand<Options>
15+
: ICommandLineOptions<Options>
1616
{
1717
public static Options Bind( ParseResult parseResult )
1818
{
1919
return new()
2020
{
2121
// NOTE: an Argument descriptor does NOT have a "Required" property
22+
// binding to a required value that is NOT present AND has no Default value provider
23+
// will generate an exception. There is no "try" semantics. One could argue that
24+
// anything with a default value provider isn't really required but it depends on perspective.
25+
// Any value that MUST be provided by the user (no default is possible) vs. any value that must
26+
// be used by the consumer (default possible if not provided) are different scenarios and each
27+
// can consider the value as "required" as things can't work without it. (That is, evaluation of
28+
// the term "required" must include a subject [that is, an answer to the "to whom/what?"].
29+
// "required" is a concept that is a dependent term and evaluation without a subject is not possible.)
30+
//
31+
// Source generator should make this CRYSTAL CLEAR.
2232
SourcePath = parseResult.GetRequiredValue( Descriptors.SourceFilePath )
2333
};
2434
}
2535

26-
public static AppControlledDefaultsRootCommand BuildRootCommand( CmdLineSettings? settings )
36+
public static AppControlledDefaultsRootCommand BuildRootCommand( CmdLineSettings settings )
2737
{
28-
return new AppControlledDefaultsRootCommand( "Kaleidoscope sample app", settings )
38+
// description is hard coded and should come from generation attribute on the "Options" type
39+
return new AppControlledDefaultsRootCommand( settings, "Kaleidoscope sample app (Chapter 9)" )
2940
{
30-
Descriptors.SourceFilePath
41+
Descriptors.SourceFilePath,
42+
43+
// Additional Options, Arguments, Sub-commands, and directives go here...
3144
};
3245
}
3346
}
@@ -38,8 +51,7 @@ file static class Descriptors
3851
internal static readonly Argument<FileInfo> SourceFilePath
3952
= new Argument<FileInfo>("SourceFilePath")
4053
{
41-
Description = "Path of the input source file [Used as ref for debug information]",
42-
HelpName = "Source file path",
54+
Description = "Path of the input source file to compile",
4355
}.AcceptExistingFileOnly();
4456
}
4557
}

src/Samples/Kaleidoscope/Chapter9/Program.cs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,10 @@
1515

1616
using static Ubiquity.NET.Llvm.Library;
1717

18+
// error CA1506: 'Main' is coupled with '41' different types from '15' different namespaces...
19+
// Total BS. Maybe at the IL level, but hardly true at the source level
20+
#pragma warning disable CA1506
21+
1822
namespace Kaleidoscope.Chapter9
1923
{
2024
public static class Program
@@ -44,7 +48,6 @@ public static int Main( string[] args )
4448
string irFilePath = Path.ChangeExtension( sourceFilePath, ".ll" );
4549
string asmPath = Path.ChangeExtension( sourceFilePath, ".s" );
4650

47-
using var rdr = File.OpenText( sourceFilePath );
4851
using var libLLVM = InitializeLLVM( );
4952
libLLVM.RegisterTarget( CodeGenTarget.Native );
5053

@@ -61,7 +64,7 @@ public static int Main( string[] args )
6164

6265
// time the parse and code generation
6366
var timer = System.Diagnostics.Stopwatch.StartNew( );
64-
var ast = parser.Parse( rdr );
67+
var ast = parser.ParseFrom( sourceFilePath );
6568
if(!errorLogger.CheckAndReportParseErrors( ast ))
6669
{
6770
Module? module = generator.Generate( ast );

src/Samples/Kaleidoscope/Kaleidoscope.Grammar/Parser.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -58,13 +58,13 @@ public LanguageLevel LanguageLevel
5858
public DynamicRuntimeState GlobalState { get; }
5959

6060
/// <inheritdoc/>
61-
public IAstNode Parse( string txt )
61+
public IAstNode? Parse( string txt )
6262
{
6363
return Parse( (txt ?? string.Empty).ToCharArray(), ParseMode.ReplLoop );
6464
}
6565

6666
/// <inheritdoc/>
67-
public IAstNode Parse( TextReader reader )
67+
public IAstNode? Parse( TextReader reader )
6868
{
6969
ArgumentNullException.ThrowIfNull( reader );
7070
return Parse( reader.ReadToEnd().ToCharArray(), ParseMode.FullSource );
@@ -80,7 +80,7 @@ internal enum ParseMode
8080
FullSource
8181
}
8282

83-
private IAstNode Parse( char[] input, ParseMode mode )
83+
private IAstNode? Parse( char[] input, ParseMode mode )
8484
{
8585
try
8686
{

src/Ubiquity.NET.CommandLine/AppControlledDefaultsRootCommand.cs

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Copyright (c) Ubiquity.NET Contributors. All rights reserved.
22
// Licensed under the Apache-2.0 WITH LLVM-exception license. See the LICENSE.md file in the project root for full license information.
33

4+
using System;
45
using System.CommandLine;
56
using System.CommandLine.Help;
67
using System.Diagnostics.CodeAnalysis;
@@ -9,17 +10,27 @@
910
namespace Ubiquity.NET.CommandLine
1011
{
1112
/// <summary>Extension of <see cref="RootCommand"/> that allows app control of defaults that are otherwise forced</summary>
13+
/// <remarks>
14+
/// This type is derived from <see cref="RootCommand"/> and offers no additional behavior beyond the construction.
15+
/// The constructor will adapt the command based on the <see cref="CmdLineSettings"/> provided. This moves the
16+
/// hard coded defaults into an app controlled domain. The default constructed settings matches the behavior of
17+
/// <see cref="RootCommand"/> so there's no distinction. This allows an application to explicitly decide the behavior
18+
/// and support of various defaults that could otherwise surprise the author/user. This is especially important when
19+
/// replacing the internal command line handling of a published app or otherwise creating a "drop-in" replacement. In
20+
/// such cases, strict adherence to back-compat is of paramount importance and the addition of default behavior is
21+
/// potentially a breaking change.
22+
/// </remarks>
1223
[SuppressMessage( "Design", "CA1010:Generic interface should also be implemented", Justification = "Collection initialization" )]
1324
public class AppControlledDefaultsRootCommand
1425
: RootCommand
1526
{
1627
/// <summary>Initializes a new instance of the <see cref="AppControlledDefaultsRootCommand"/> class.</summary>
1728
/// <param name="description">Description of this root command</param>
1829
/// <param name="settings">Settings to apply for the command parsing</param>
19-
public AppControlledDefaultsRootCommand( string description = "", CmdLineSettings? settings = null )
30+
public AppControlledDefaultsRootCommand( CmdLineSettings settings, string description = "" )
2031
: base( description )
2132
{
22-
settings ??= new CmdLineSettings();
33+
ArgumentNullException.ThrowIfNull(settings);
2334

2435
// RootCommand constructor already adds HelpOption and VersionOption so remove them
2536
// unless specified by caller.

0 commit comments

Comments
 (0)