Set disableoutputsubstitution to 'true' if used

[arminsabouri]

I noticed this when I was writing tests for my new multiparty optional param. In the sender if we are setting disableoutputsubstitution we set it to a '1' value. However in the reciever we are parsing for 'true' string value if its set.

[arminsabouri]

While im in here I might push another commit to just test different permutations of urls. Lmk if you guys prefere that in a different PR, a different commit or just part of the first commit

[DanGould]

Great catch

I believe the spec requires use of true rather than 1, so maybe this change needs to happen to the opposite location

[coveralls]

Pull Request Test Coverage Report for Build 12795364998

Details

• 11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.1%) to 60.766%

---

Totals
Change from base Build 12791012687:	0.1%
Covered Lines:	2935
Relevant Lines:	4830

---

💛 - Coveralls

[arminsabouri]

Great catch

I believe the spec requires use of true rather than 1, so maybe this change needs to happen to the opposite location

You got it!

[DanGould]

Also fyi, some URL permutation tests live in uri/mod.rs and uri/url_ext.rs. Were you trying to do a different kind of testing?

[DanGould]

utACK de2b625

Clear commit message 🎆

Would be nice to have some regression test so this doesn't get reintroduced accidentally

[spacebear21]

tACK de2b625

[DanGould]

tACK de2b625 — @spacebear21

tACK meaning you tested this behavior specifically or that you ran the test suite? By convention going forward I'd like tACK to mean the former

[arminsabouri]

s set to '1' if the sender decides to disable output substitution which then the reciever will just
ignore.

I dont see any test coverage over reciever optional params. I was planning on writing some tests to cover those. Perhaps that belongs in a different PR.

Would be nice to have some regression test so this doesn't get reintroduced accidentally

Yeah let me see what I can do. It should be easy to write a regression test for this

[spacebear21]

I just ran the test suite - good point moving forward.

[DanGould]

Merging to keep the queue flowing. Hoping to see a regression test but no expectation there since not excluding one is no regression in itself (edit: afaict).

[arminsabouri]

Merging to keep the queue flowing. Hoping to see a regression test but no expectation there since not excluding one is no regression in itself (edit: afaict).

I did end up including a test to ensure this doesnt get accidentaly re-introduced.

Would you have written a different test or expected something different?

[DanGould]

Oh you did! my bad, I didn't realize you pushed more code after I ACKed, we don't have the protection on that prevents that merge by default.

This is what I would expect for a unit test, though send::serialize_url does make me wonder whether or not optional_parameters should define that function, and if it is truly a receive module or a general one

[arminsabouri]

Oh you did! my bad, I didn't realize you pushed more code after I ACKed, we don't have the protection on that prevents that merge by default.

This is what I would expect for a unit test, though send::serialize_url does make me wonder whether or not optional_parameters should define that function, and if it is truly a receive module or a general one

All good! Yeah we can add a branch protection to dismiss review on force updates.

Yeah it would make sense to me if this url was more general not belonging to either the sender or reciever. Instead a shared type

[DanGould]

I set "Require approval of the most recent reviewable push" restriction on master

I believe url is general but optional query parameters that get sent are not. optional parameters are not abstracted in the sender, as demonstrated by the change was in send/mod.rs, they're only abstracted in the receiver/optional_parameters.rs.