commandlineparser / commandlineparser/commandline

Proposal for change: Remove raising exception when Copyright or company assembly attributes are null and set with default values

オープン
#403 コメント 3 件 リアクション 2 件 担当者 0 名 GitHub で見る
主要言語
C#
スター
4.8k
フォーク
478
PR マージ指標
30日以内にマージされた PR はありません

説明

**Problem**
Test fail in Xunit or Nunit framework in net4x when Copyright or Company assembly attributes are null

**Problem Analysis**
The problem is existing since version 2.3 and now in v2.4.3 but the behavior is different based on the test project is net4x.
The test fail for the test cases that test --help or --version or even errors for bad options in the commandline
See this [issue ](https://github.com/commandlineparser/commandline/issues/354) and [issue ](https://github.com/commandlineparser/commandline/issues/389)
and [this ](https://github.com/nunit/nunit/issues/3084) in NUnit project

These issues can't be detected by the current unit test project because unit test uses the method `SetAttributeOverride' and bypass the `EntryAssemply`

The CommandLineParser in Production is running Ok when reading AssemblyAttributes copyright and company, but in Test project in Full Framework net4x or latter it cause the failure of test when reading assembly attributes.
The test is OK in netcoreapp2.x

In production the entryAssemply is the console project itself which have copyright and company assembly attributes
but in test , the executer is the testhost framework itself which has null values for copyright and company assemply attributes.

This because the CommandlineParser raise Exception when CopyRight/Company are null see [this](https://github.com/commandlineparser/commandline/blob/master/src/CommandLine/Text/CopyrightInfo.cs#L120)

**Behavior in v2.3**
in net45x or latter: Assembly.GetExecutingAssembly();
in netstandard1.5 or latter: Assembly.GetEntryAssembly()

private static Assembly GetExecutingOrEntryAssembly()
{
var assembly = Assembly.GetEntryAssembly();

#if !NETSTANDARD1_5
assembly = assembly ?? Assembly.GetExecutingAssembly();
#endif

return assembly;
}

**Behavior in v2.4.3**
in netstandrad2.x :Assembly.GetEntryAssembly();

private static Assembly GetExecutingOrEntryAssembly()
{
return Assembly.GetEntryAssembly();
}

**Advantage of removing raising exception**

- The unit test don't fail
- No side effect at all for the flow of the CommandLineParser.
- Even, in test, the values of copyright and company is not valid in unit test and they are the values in hosttest assemblies and currently CommandlineParser pass artificial values for them in unit test.
- These values are different based on the version of testhost used and causes troubles to the unit test.
- Setting default values is better than firing exception.

**Suggested Solution**
PR can modify [CopyrightInfo.cs](https://github.com/commandlineparser/commandline/blob/master/src/CommandLine/Text/CopyrightInfo.cs) and [Headinginfo.cs](https://github.com/commandlineparser/commandline/blob/master/src/CommandLine/Text/HeadingInfo.cs) to remove exception and set default values.

1. When reading copyright and it's null , don't fire exception and set a default value like "copyright @currentYear"
2. When reading Company and it's null , don't fire exception and set a default value like "Copmpany @productName"
3. Modify the method GetExecutingOrEntryAssembly as described below

**Consideration for Unit Test**

- Test project is converted to multi-target project netcoreapp2.0 and net461
- Avoid using the method [SetAttributeOverride](https://github.com/commandlineparser/commandline/blob/master/src/CommandLine/Infrastructure/ReflectionHelper.cs#L30) because it bypass the the method `GetExecutingOrEntryAssembly` which is under test.

Proof of Concept
POC can be done for evaluating this change.

**Update**
Also, the following code in [ReflectionHelper](https://github.com/commandlineparser/commandline/blob/master/src/CommandLine/Infrastructure/ReflectionHelper.cs#L97):

private static Assembly GetExecutingOrEntryAssembly()
{
return Assembly.GetEntryAssembly();
}
can be modified to:

private static Assembly GetExecutingOrEntryAssembly()
{
// this insures that assembly will not be null
var assembly = Assembly.GetEntryAssembly() ?? Assembly.GetCallingAssembly()
return assembly
}

[Assembly.GetCallingAssembly](https://docs.microsoft.com/en-us/dotnet/api/system.reflection.assembly.getcallingassembly?view=netframework-4.7.2) Returns the Assembly of the method that invoked the currently executing method

**POC**
I did the modification of GetExecutingOrEntryAssembly as described in `(GetExecutingOrEntryAssembly)` and modified the test project to multi-target netcoreapp2.0 and net461/net472 and all tests (334 test case) pass successfully in both targets. Also I test the issue [389 ](https://github.com/commandlineparser/commandline/issues/389#issuecomment-457400442) and no exception is fired

コントリビューションガイド

このリポジトリのコントリビューションガイドは索引されていません

評価

この issue はまだ評価されていません。

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。