Remove obsolete `Concat` method

pflajszer commented 1 year ago

Removed an obsolete Concat method
Updated README with new info on the methods removal.

Closes #993

pflajszer commented 1 year ago

hmmm, I think the build failed because of the API definition changing between v 3.X and 4, but that might not be necessarily the problem with the PR. Can anyone assist with this, please?

pflajszer commented 1 year ago

Due to the breaking changes from v3.4.1 to v.4.0.0, the package validation fails because we have to use a previous version to validate. In MoreLinq.csjproj: <PackageValidationBaselineVersion>3.4.1</PackageValidationBaselineVersion>

How do you normally deal with breaking changes? It seems you can introduce an error suppression, but I wouldn't want to make calls like this without some discussion here.

i.e:

<PropertyGroup>
  <GenerateCompatibilitySuppressionFile>true</GenerateCompatibilitySuppressionFile>
  <CompatibilitySuppressionFilePath>ApiCompatSuppressions.xml</CompatibilitySuppressionFilePath>
</PropertyGroup>

[source]

pflajszer commented 1 year ago

How do you normally deal with breaking changes? It seems you can introduce an error suppression, but I wouldn't want to make calls like this without some discussion here.

It would be as described in the documentation.

I wouldn't add GenerateCompatibilitySuppressionFile in the MoreLinq.csproj file though, because that would defeat the whole purpose of the validation in the first place. It should be done on an as-needed basis from the CLI when the breaking changes are flagged and indeed intended:
dotnet pack -p:GenerateCompatibilitySuppressionFile=true
The CompatibilitySuppressionFilePath is not currently documented so I wouldn't add it either. The default named file (MoreLinq/CompatibilitySuppressions.xml) was already added with PR #974.

I think you'll also need to run build.cmd/build.sh locally to update the generated extensions (MoreLinq/Extensions.g.cs) otherwise the CI build will fail in spite of the above fix.

I think we should then be in a position to merge. Thanks!

Thanks. We're getting there!

running dotnet pack -p:GenerateCompatibilitySuppressionFile=true updated the MoreLinq/CompatibilitySuppressions.xml file
running build.cmd locally didn't do anything to Extensions.g.cs, but it seems to be generating that fine looking at the build log (below). The build command itself fails with error IDE0251:

Build output

PS P:\source\repos\MoreLINQ> ./build.cmd
  Determining projects to restore...
  All projects are up-to-date for restore.
Tool 'dotnet-t4' (version '2.3.0') was restored. Available commands: t4
Tool 'dotnet-reportgenerator-globaltool' (version '5.1.11') was restored. Available commands: reportgenerator
Tool 'meziantou.framework.nugetpackagevalidation.tool' (version '1.0.9') was restored. Available commands: meziantou.validate-nuget-package

Restore was successful.
Generating extensions wrappers (MoreLinq\Extensions.g.cs)...Done.
Generating extensions wrappers (MoreLinq\Extensions.ToDataTable.g.cs)...Done.
MSBuild version 17.6.1+8ffc3fe3d for .NET
  Determining projects to restore...
  All projects are up-to-date for restore.

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:00.48
MSBuild version 17.6.1+8ffc3fe3d for .NET
  MoreLinq.ExtensionsGenerator -> P:\source\repos\MoreLINQ\bld\ExtensionsGenerator\bin\Debug\net7.0\MoreLinq.ExtensionsGenerator.dll
  MoreLinq -> P:\source\repos\MoreLINQ\MoreLinq\bin\Debug\netstandard2.1\MoreLinq.dll
  MoreLinq -> P:\source\repos\MoreLINQ\MoreLinq\bin\Debug\net6.0\MoreLinq.dll
  MoreLinq -> P:\source\repos\MoreLINQ\MoreLinq\bin\Debug\netstandard2.0\MoreLinq.dll
P:\source\repos\MoreLINQ\MoreLinq.Test\ToDataTableTest.cs(190,33): error IDE0251:  [P:\source\repos\MoreLINQ\MoreLinq.Test\MoreLinq.Test.csproj::TargetFramework=net471]
P:\source\repos\MoreLINQ\MoreLinq.Test\ToDataTableTest.cs(190,33): error IDE0251:  [P:\source\repos\MoreLINQ\MoreLinq.Test\MoreLinq.Test.csproj::TargetFramework=net7.0]
P:\source\repos\MoreLINQ\MoreLinq.Test\ToDataTableTest.cs(190,33): error IDE0251:  [P:\source\repos\MoreLINQ\MoreLinq.Test\MoreLinq.Test.csproj::TargetFramework=net6.0]

Build FAILED.

P:\source\repos\MoreLINQ\MoreLinq.Test\ToDataTableTest.cs(190,33): error IDE0251:  [P:\source\repos\MoreLINQ\MoreLinq.Test\MoreLinq.Test.csproj::TargetFramework=net471]
P:\source\repos\MoreLINQ\MoreLinq.Test\ToDataTableTest.cs(190,33): error IDE0251:  [P:\source\repos\MoreLINQ\MoreLinq.Test\MoreLinq.Test.csproj::TargetFramework=net7.0]
P:\source\repos\MoreLINQ\MoreLinq.Test\ToDataTableTest.cs(190,33): error IDE0251:  [P:\source\repos\MoreLINQ\MoreLinq.Test\MoreLinq.Test.csproj::TargetFramework=net6.0]
    0 Warning(s)
    3 Error(s)

Time Elapsed 00:00:02.20
PS P:\source\repos\MoreLINQ>

After a little investigation, adding a readonly to MoreLINQ\MoreLinq.Test\ToDataTableTest.cs(190,33) does the job (refer to this post) and the build completes successfully, but no actual changes are made. I've tried that on master and it also happens, so perhaps a candidate (and a possible fix too) for another issue?

codecov[bot] commented 1 year ago

Codecov Report

Merging #1002 (8965d5c) into master (852fb1c) will not change coverage. The diff coverage is 100.00%.

:exclamation: Current head 8965d5c differs from pull request most recent head 0e88e96. Consider uploading reports for the commit 0e88e96 to get more accurate results

@@           Coverage Diff           @@
##           master    #1002   +/-   ##
=======================================
  Coverage   92.59%   92.59%           
=======================================
  Files         113      113           
  Lines        3430     3430           
  Branches     1054     1054           
=======================================
  Hits         3176     3176           
  Misses        191      191           
  Partials       63       63

Impacted Files	Coverage Δ
MoreLinq/Append.cs	`100.00% <ø> (ø)`
MoreLinq/ToDataTable.cs	`96.80% <100.00%> (ø)`

... and 1 file with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

pflajszer commented 1 year ago

Thanks for your contribution, @pflajszer!

I'm just getting comfortable contributing to OS projects, so that looks like a reasonable one to start with.

Hope this was a good start and look forward to further collaborations.

That's a lot for the guidance! It definitely gives me a confidence boost in the open-source world! I look forward too!

morelinq / MoreLINQ

Remove obsolete `Concat` method #1002

Codecov Report