Skip to content

Conversation

@EdwinEngelen
Copy link
Contributor

For a warning free experience when consuming the NuGet package Zlib.Portable into a .NET Standard or .NET (Core) project, it's migrated to .NET Standard.

There is one failing test, GzipTrailerValidation.SystemGzipStreamWithBadTrailer_DoesNotThrowWhenReadToEnd. I except that this test failed before.

Version bumped to 1.12.0.0.

If everything is fine, can you also create and upload the NuGet package? I did not configure the new NuGet packaging properties of MSBuild.

Thanks!

@robertmclaws
Copy link
Collaborator

Thanks for this! I'll work on getting the pipeline updated tonight so a new build can go out.

Did you review this PR to see if any of the changes it in are still needed?

Thanks!

@EdwinEngelen
Copy link
Contributor Author

EdwinEngelen commented Jul 23, 2021

I didn't review the other PR, because haven't looked for other PR's. Sorry for that.

  • My cached 1.11.0 version isn't signed. The 'Release-signed' configuration is not working anymore. Should we keep the strong signing?
  • PR Update to support multi targeting for .NET Core #10 did some cleanup and warning removal. I intentionally did not make these 'irrelevant' changes. Should we clean things up?
  • PR Update to support multi targeting for .NET Core #10 uses multi-targeting, .NETPortable,Version=v4.0,Profile=Profile328 and netstandard2.0. I used only netstandard2.0. For compatibility with older platforms such as Silverlight I might be better to target both. What do you think?

@EdwinEngelen
Copy link
Contributor Author

I tried to do some cleaning up, but there are quite some things to be improved. Unused usings removal, spacing fixes, unused constants, fields that can be made readonly, 'new' modifier for methods hiding base class methods, ... I propose to NOT do the cleaning up, because the library is stable and ok, hard to review all the changes, risk of introducing bugs, pollution of code history, and so on.

@robertmclaws
Copy link
Collaborator

No worries. I was mostly referring to the changes in Path.cs and FileOutputStream.cs.

@EdwinEngelen
Copy link
Contributor Author

EdwinEngelen commented Jul 24, 2021

No worries. I was mostly referring to the changes in Path.cs and FileOutputStream.cs.

  • Path.cs:
    Don't need these changes. Are for warning removal only.
  • ParallelDeflateOutputStream.cs:
    Don't need these changes. override was added because of a warning. But that changes the behavior. new should be used to have the same behavior.
  • ZlibBaseStream.cs:
    Same as ParallelDeflateOutputStream.cs

My conclusion: we don't need these changes and can leave the warnings as is.

@conradmicallef, do you agree?

@robertmclaws robertmclaws merged commit 86b16d4 into CloudNimble:master Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants