blachniet commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r523545792
##########
File path: lang/csharp/common.props
##########
@@ -45,6 +45,7 @@
<ItemGroup>
<None Include="$(MSBuildThisFileDirectory)\LICENSE" Pack="true"
Visible="false" PackagePath=""/>
+ <None Include="$(MSBuildThisFileDirectory)\icon.png" Pack="true"
PackagePath=""/>
Review comment:
Could we use the `avro-logo.png` in
[avro/doc/src/resources/images/](https://github.com/apache/avro/tree/master/doc/src/resources/images)
rather than adding another copy of it here?
##########
File path: lang/csharp/src/apache/main/Schema/SchemaNormalization.cs
##########
@@ -157,7 +157,7 @@ private static StringBuilder Build(IDictionary<string,
string> env, Schema s, St
{
if (!firstTime)
{
- o.Append(",");
+ o.Append(',');
Review comment:
Why all the changes from double to single quotes? Is this necessary? Was
there a warning or code hint that was suggesting this?
##########
File path: lang/csharp/src/apache/test/Avro.test.csproj
##########
@@ -17,8 +17,8 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
- <TargetFrameworks
Condition="'$(OS)'!='Windows_NT'">netcoreapp3.1</TargetFrameworks>
- <TargetFrameworks
Condition="'$(OS)'=='Windows_NT'">net461;netcoreapp3.1</TargetFrameworks>
+ <TargetFrameworks
Condition="'$(OS)'!='Windows_NT'">net5.0</TargetFrameworks>
+ <TargetFrameworks
Condition="'$(OS)'=='Windows_NT'">net461;net5.0</TargetFrameworks>
Review comment:
It may be good for us to continue testing against .NET Core 3.1, just in
case there was some compatibility issue between the two.
```suggestion
<TargetFrameworks
Condition="'$(OS)'!='Windows_NT'">netcoreapp3.1;net5.0</TargetFrameworks>
<TargetFrameworks
Condition="'$(OS)'=='Windows_NT'">net461;netcoreapp3.1;net5.0</TargetFrameworks>
```
##########
File path: lang/csharp/src/apache/test/Reflect/TestFromAvroProject.cs
##########
@@ -189,7 +189,8 @@ public Z SerializeDeserialize(Z z)
catch (Exception ex)
{
Console.WriteLine(ex.ToString());
- throw ex;
+ // Note: Re-throwing caught exception changes stack information
+ throw;
Review comment:
Great catch 😄 ! I think you can safely remove the comment here.
```suggestion
throw;
```
##########
File path: lang/csharp/src/apache/main/Generic/GenericRecord.cs
##########
@@ -239,7 +239,7 @@ public override string ToString()
sb.Append(contents[field.Pos]);
sb.Append(", ");
}
- sb.Append("}");
+ sb.Append('}');
Review comment:
Is this change necessary?
##########
File path: lang/csharp/src/apache/main/Schema/SchemaName.cs
##########
@@ -64,7 +64,7 @@ public SchemaName(String name, String space, String encspace)
this.EncSpace = encspace; // need to save enclosing
namespace for anonymous types, so named types within the anonymous type can be
resolved
}
#pragma warning disable CA1307 // Specify StringComparison
- else if (name.IndexOf('.') == -1)
+ else if (!name.Contains("."))
Review comment:
Just out of curiosity, did this come up in a code hint or warning?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
