Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate CompressionLevel value is defined in BrotliStream ctor #57561

Merged
merged 5 commits into from
Nov 1, 2021

Conversation

i3arnon
Copy link
Contributor

@i3arnon i3arnon commented Aug 17, 2021

Fix #46595

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 17, 2021
@ghost
Copy link

ghost commented Aug 17, 2021

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix #46595

Author: i3arnon
Assignees: -
Labels:

area-System.IO.Compression

Milestone: -

@stephentoub stephentoub added this to the 7.0.0 milestone Aug 17, 2021
@stephentoub stephentoub added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Aug 17, 2021
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Aug 17, 2021
@ghost
Copy link

ghost commented Aug 17, 2021

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

@i3arnon
Copy link
Contributor Author

i3arnon commented Aug 19, 2021

2. Ask a committer to mail the .NET Breaking Change Notification DL.

@stephentoub Can you please send the breaking change suggestion to the DL? dotnet/docs#25665

@stephentoub
Copy link
Member

if it's merged

@carlossanlop
Copy link
Member

The CI results are already deleted. I want to make sure we don't have unit test failures. Closing and reopening to re-trigger the CI (same with the other related PR).

@carlossanlop carlossanlop reopened this Oct 5, 2021
Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see the comment from @scalablecory here: #57561 (comment)

Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm signing off, although I added the last commits to address the suggestion and to add tests. @jozkee @adamsitnik @jeffhandley @stephentoub would you mind giving us an additional sign-off if you agree with the change?

@carlossanlop carlossanlop merged commit 1d08e15 into dotnet:main Nov 1, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 2, 2021
@i3arnon i3arnon deleted the compression-level-validation branch January 12, 2022 13:20
@ericstj
Copy link
Member

ericstj commented Oct 4, 2022

@carlossanlop were you planning to create a breaking change document for this?

@eerhardt
Copy link
Member

eerhardt commented Oct 27, 2022

We should also consider updating this blog: https://devblogs.microsoft.com/dotnet/introducing-support-for-brotli-compression/

It explicitly calls out this behavior as a feature:

As mentioned earlier, Brotli supports 11 levels of compression quality aside from the standard Optimal, Fastest and NoCompression levels. You can pass in the level to the constructor by casting the desired level directly to CompressionLevel, like this:

BrotliStream brotli = new BrotliStream(baseStream,
                                       CompressionMode.Compress,
                                       leaveOpen,
                                       bufferSize,
                                       (CompressionLevel)5); // Use level 5

I stumbled on it from searching for "brotli compression kestrel".

cc @terrajobst

@stephentoub
Copy link
Member

I've removed it.

@jozkee
Copy link
Member

jozkee commented Nov 16, 2022

breaking change doc created dotnet/docs#32620.

@jozkee jozkee removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO.Compression breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revisit BrotliStream CompressionLevel values
8 participants