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

Check if request headers exist #128

Merged
merged 4 commits into from
Oct 6, 2021
Merged

Check if request headers exist #128

merged 4 commits into from
Oct 6, 2021

Conversation

PhyxionNL
Copy link
Contributor

These headers are not guaranteed.

These headers are not guaranteed.
@PhyxionNL
Copy link
Contributor Author

As a side note, @Shazwazza, how useful is the IE6 check still? Maybe remove it? I doubt anyone uses that ancient browser anymore.

@Shazwazza
Copy link
Owner

Yep we can remove that check :) can you update the PR with the change?

@PhyxionNL
Copy link
Contributor Author

Yep we can remove that check :) can you update the PR with the change?

Done :)


CompressionType parsed = CompressionType.Parse(encoding);

// Brotli is typically last in the accept encoding header.
if (parsed == CompressionType.Brotli)
if (parsed.Equals(CompressionType.Brotli))
Copy link
Owner

Choose a reason for hiding this comment

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

Do we want to do any invariant checking for Equals here or no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is a struct. I changed it to Equals instead of == because VS was warning about a not implemented == operator (seems like a VS bug as CompressionType implements the == operator as well).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Shazwazza Reverted these changes, if you could merge and release a new version on NuGet that'd be great 👍

Copy link

@eladmarg eladmarg left a comment

Choose a reason for hiding this comment

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

looks fine.

@Shazwazza Shazwazza merged commit 0ba3ae5 into Shazwazza:master Oct 6, 2021
@Shazwazza Shazwazza added this to the 4.0.2 milestone Oct 6, 2021
@PhyxionNL PhyxionNL deleted the fix-headers branch October 6, 2021 16:21
@Shazwazza
Copy link
Owner

@eladmarg will this resolve #130 ?

@Shazwazza
Copy link
Owner

Ahh, you already said that. I think it will since it's trying to read from the dictionary correctly.

@PhyxionNL
Copy link
Contributor Author

Yes, this fixes that problem, it's the same issue I had. The User-Agent header I can understand missing but I also had some Chrome browsers not sending Accept-Encoding for whatever reason.

@Shazwazza
Copy link
Owner

Cool, i've just published the release 👍

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.

None yet

3 participants