Merge YAXLib.Redux #120

NKnusperer · 2021-03-02T08:00:16Z

No description provided.

- Updates build infrastructure to .NET 5.0 - YAXLib now targets netstandard2.0 - Testsuite is green when targeting net48 and net5.0 - DemoApplication is now a NET 5 Windows Forms app - General cleanup - Normalized line endings - Ignore .idae - Published NuGet package

…lib.redux

axunonb · 2021-03-02T09:36:17Z

Very good, thanks! Few questions re current PR:

Should we have target frameworks net461, netstandard2.0, net5.0 with corresponding tests under net461, netcoreapp3.1, net5.0?
Would a reference to Microsoft.SourceLink.GitHub be a good thing for a new release? (It's included in the current master)?
Should C# language version (v9?) be included in csproj?
appveyor.yml is still on msbuild. You mentioned moving to dotnet, which makes sense
Thinking of a semver v3 release: Should members marked as obsolete be already removed?
Publish more platforms for Linux (as Julian proposed)?
Current master includes a ReSharper "full code clean-up" (and updated file headers). Would this make sense to you?

NKnusperer · 2021-03-02T10:09:11Z

net461 does not really support netstandard2.0, I will just quote the Microsoft documentation here about this:

While NuGet considers .NET Framework 4.6.1 as supporting .NET Standard 1.5 through 2.0, there are several issues with consuming .NET Standard libraries that were built for those versions from .NET Framework 4.6.1 projects. For .NET Framework projects that need to use such libraries, we recommend that you upgrade the project to target .NET Framework 4.7.2 or higher.

So technically we also could test using net472 if we really want to. Not sure about this. Same for netcoreapp3.1.
Personally I do not like the idea of allocating resources to support platforms which will be EOL soon, but this is open for discussion.

I like Source Link but never have implemented it, if the current implementation is working then we should use this.
LangVersion is set to latest which is bound to the current SDK version specified in the global.json file so it is in fact already v9.

YAXLib/Directory.Build.props

Line 12 in fcd2ede

<LangVersion>latest</LangVersion>

YAXLib/global.json

Line 3 in fcd2ede

"version": "5.0.103"
You're right, I missed this part (now implemented).
I noticed this but I'am not sure what this was used for and if it is still relevant. For me it would be fine to remove this.
There is no need to publish for different platforms, everything should work with a single netstandard2.0 build artifact.
Didn't noticed this, have to review this.

axunonb · 2021-03-02T14:54:55Z

Re 7 there is also an existing .editorconfig

304NotModified

Personally I have difficulties to accept this pr as there are too much changed files imo. (not only newlines). I'm not sure if we could review 144 changed files. Also not all changes seems to be strictly needed.

But if @axunonb think this is ok, I will trust that.

Ps

like Source Link but never have implemented it, if the current implementation is working then we should use this.

I've done that multiple times and I could check it. Maybe create an issue for it so we don't forget?

304NotModified · 2021-03-02T23:29:55Z

YAXLib/StringUtils.cs

+                // http://msdn.microsoft.com/en-us/library/system.xml.linq.xname.aspx
+
+                // Leave namespace part alone, refine localname part.
+                int closingBrace = elemName.IndexOf( '}' );


I don't think these changes are needed (removed var)

It's poluting the pr

Totally agree, it was just a personal preference to change this because I don't like the idea of "hiding" types when they are not explicitly declared both on the left or right hand side and I have to use the IDE to get this information.
To resolve this I would have to redo the all of this from a vanilla version of YAXLib. Do we want this?

Let's discuss the future code base outside the pr

To resolve this I would have to redo the all of this from a vanilla version

ReSharper could change to var syntax style across the solution.

304NotModified · 2021-03-02T23:37:07Z

Would a reference to Microsoft.SourceLink.GitHub be a good thing for a new release? (It's included in the current master)?

Yes, but it should be private.

NKnusperer · 2021-03-03T08:01:44Z

@axunonb the .editorconfig is already included.
Also I added source link as seen in #108: 253a43f
However I'm not sure if this is already enough because I think we are missing the ContinuousIntegrationBuild as described here:
https://devblogs.microsoft.com/dotnet/producing-packages-with-source-link/#deterministic-builds
Does the GITHUB_ACTIONS apply for appveyor too?

304NotModified · 2021-03-03T10:14:34Z

However I'm not sure if this is already enough because I think we are missing the ContinuousIntegrationBuild as described here:
https://devblogs.microsoft.com/dotnet/producing-packages-with-source-link/#deterministic-builds
Does the GITHUB_ACTIONS apply for appveyor too?

Let do this in another PR? Related: NLog/NLog.Web#639

NKnusperer · 2021-03-04T08:28:33Z

See #122.

NKnusperer added 4 commits February 5, 2021 11:48

Merge remote-tracking branch 'upstream/master' into fetaure/merge-yax…

30599ce

…lib.redux

Updated appveyor.yml

ec01381

Target tests for net5.0 and net48

fcd2ede

NKnusperer requested a review from axunonb March 2, 2021 08:00

NKnusperer mentioned this pull request Mar 2, 2021

YAXLib vNext #116

Closed

NKnusperer force-pushed the fetaure/merge-yaxlib.redux branch 2 times, most recently from 7304539 to 6df30dd Compare March 2, 2021 10:28

Use dotnet build instead of msbuild

d705230

NKnusperer force-pushed the fetaure/merge-yaxlib.redux branch from 6df30dd to d705230 Compare March 2, 2021 10:29

304NotModified reviewed Mar 2, 2021

View reviewed changes

Added Microsoft.SourceLink.GitHub

253a43f

axunonb requested a review from 304NotModified March 3, 2021 18:00

NKnusperer mentioned this pull request Mar 4, 2021

Merge YAXLib.Redux v2 #122

Merged

NKnusperer closed this Mar 4, 2021

304NotModified deleted the fetaure/merge-yaxlib.redux branch March 21, 2021 21:43

Merge YAXLib.Redux #120

Merge YAXLib.Redux #120

Uh oh!

Conversation

NKnusperer commented Mar 2, 2021

Uh oh!

axunonb commented Mar 2, 2021

Uh oh!

NKnusperer commented Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

axunonb commented Mar 2, 2021

Uh oh!

304NotModified left a comment

Choose a reason for hiding this comment

Uh oh!

304NotModified Mar 2, 2021

Choose a reason for hiding this comment

Uh oh!

NKnusperer Mar 3, 2021

Choose a reason for hiding this comment

Uh oh!

axunonb Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axunonb Mar 3, 2021

Choose a reason for hiding this comment

Uh oh!

304NotModified commented Mar 2, 2021

Uh oh!

NKnusperer commented Mar 3, 2021

Uh oh!

304NotModified commented Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NKnusperer commented Mar 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

NKnusperer commented Mar 2, 2021 •

edited

Loading

axunonb Mar 3, 2021 •

edited

Loading

304NotModified commented Mar 3, 2021 •

edited

Loading