hack-router icon indicating copy to clipboard operation
hack-router copied to clipboard

make HEAD/GET method rewrite configurable

Open azjezz opened this issue 5 years ago • 3 comments

closes #15

azjezz avatar Feb 08 '20 20:02 azjezz

Seems reasonable but Fred has stronger opinions about this so I'll leave it to him. Some potential minor concerns:

  • if we later decided on a different solution and removed the flag, it would break backwards compatibility so we should be reasonably sure we want to stick to this solution
  • boolean flags can be confusing (as part of a public API, that is -- they're fine in private methods etc.), for someone reading the code it's impossible to know what new SomeClass(false) means, consider replacing that with either
    • a method call: (new SomeClass())->disableHeadRewrite()->...
    • private constructor + pair of static "factory" methods: SomeClass::withHeadRewrite()->... and SomeClass::withoutHeadRewrite()->...

jjergus avatar Feb 10 '20 16:02 jjergus

I'm fine with the intent of this diff, but not the bool argument; I'm fine with a shape argument, or the instance methods.

The problem with static factory methods is it's not scalable if we want to add other options in the future.

fredemmott avatar Feb 10 '20 18:02 fredemmott

updated to use options shape :+1:

azjezz avatar Feb 10 '20 20:02 azjezz