Skip to content
This repository has been archived by the owner on Apr 6, 2021. It is now read-only.

add preserveHeaderCase argument #65

Closed
wants to merge 3 commits into from

Conversation

zichangg
Copy link

@zichangg zichangg commented Feb 3, 2020

An upcoming SDK release will add a named argument which must be included for any class which implements the interface. Pre-emptively add the argument to the signature so that we have a version of this package which is forward compatible with the SDK. We will properly forward the argument once we have an SDK release with the change.

dart-lang/sdk#39657

@zichangg
Copy link
Author

zichangg commented Feb 5, 2020

Hi Nate,
Can you review this change?

Copy link
Contributor

@natebosch natebosch left a comment

Choose a reason for hiding this comment

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

Can we add tests of the new behavior?

} else {
name = name.toLowerCase();
_originalHeaderName[name] = name;
}
_headers.remove(name);
Copy link
Contributor

Choose a reason for hiding this comment

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

We'd want the lowercase name here even if preserveHeaderCase is true.

Copy link
Author

Choose a reason for hiding this comment

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

Actually I'm not confident with this behavior change.
Is this the behavior this package wants? If not, I just need to update the signature. Or I can do that whenever there is a request.
What do you think?

# 0.10.0

* Preparation for [HttpHeaders change]. Update signature of `set()` to match
new signature of `HttpHeaders`. The parameter is not yet forwarded and
Copy link
Contributor

Choose a reason for hiding this comment

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

This description doesn't make sense here. We are changing behavior based on this.

@natebosch
Copy link
Contributor

Ah, I didn't realize this was a test utility only...

@natebosch
Copy link
Contributor

To shorten the cycle time, let's run with #66

We don't need anything more complex than that since it's a test utility so we only care about the signature, not the behavior of passing true.

@zichangg
Copy link
Author

zichangg commented Feb 5, 2020

Sounds good. Thanks for the review.

@zichangg zichangg closed this Feb 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

3 participants