[WIP] Tweak Request API behavior for multi-value parameters & headers #1521
Conversation
|
💖 Thanks for opening this pull request! 💖 To help make this a smooth process, please be sure you have first read the |
| @@ -97,7 +97,13 @@ describe('request.*', () => { | |||
| { name: 'hello', value: 'world' }, | |||
| { name: 'Content-Type', value: 'application/json' }, | |||
| ], | |||
| parameters: [{ name: 'foo', value: 'bar' }, { name: 'message', value: 'Hello World!' }], | |||
| parameters: [ | |||
| { name: 'foo', value: 'bar' }, | |||
There was a problem hiding this comment.
Can we change the test data to the following?
{ name: 'foo', value: 'bar' },
{ name: 'multi', value: 'a' },
{ name: 'message', value: 'Hello World!' },
{ name: 'multi', value: 'b' },
{ name: 'multi', value: 'c' },
{ name: 'trailing', value: 't' },At first glance, it looks like your code does not preserve the position when using setParameter when multiple exist.
What do you think the behavior should be in this case?
- Never preserve position
- Replace first occurrence and delete the rest
- Replace last occurrence and delete the rest
Let me know what you think 🤔
There was a problem hiding this comment.
Honestly, I have no idea, and I'd be interested to hear arguments for any of them. In the meantime, maybe it'd be best to explicitly:
- not make a decision here
- say so in the API docs (as in something like "Setting a parameter may change its position in the list. This behaviour is currently undefined, so don't depend on it as it may change in future.")
|
|
||
| // setHeader() | ||
| // setParameter() |
|
Sorry that this is taking a while! I have a load of work on right now, but I'd like to get back to this when I can. In the meantime, anyone who's interested should feel free to continue without me. |
|
@gschier Talking of documentation (as I do in one of the threads above): I'm happy to update the API docs to go with this change, but I don't see them anywhere. Are they open to editing in some way, or should I leave that to you? |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Closes #1518, with the changes proposed by @gschier (unless discussion takes us elsewhere):