search

microsoft / typed-rest-client

Node Rest and Http Clients with typings for use with TypeScript
Other
675 stars 118 forks source link

Makes RestClient._processResponse protected #200

Closed AnthonyDiSanti closed 4 years ago

AnthonyDiSanti commented 4 years ago

Resolves #199

stephenmichaelf commented 4 years ago

Thanks for contributing to the lib, @AnthonyDiSanti ! Generally what consumers do here is wrap the code and throw themselves on 404 if they need to. We would prefer to not expose this method if we can avoid it.

Would that work for you?

AnthonyDiSanti commented 4 years ago

Thank you for the recommendation, @stephenmichaelf . That's the approach I'm taking today, but it requires wrapping each public method. I find that reflection tends to throw off the vscode typescript parser, reducing its usefulness, so I'm maintaining the list of public methods in my derived class. The alternative implementation would be include this 404 boilerplate everywhere I use the class. In either case, wrapping _processResponse directly is a cleaner solution.

I understand why you wouldn't want to make this method public, but derived classes would greatly benefit from access to it. If the team has strong feelings that this method should not be exposed, no problem, I very much appreciate your work maintaining this and I'll continue using the workaround. However, I really do think this would be an improvement.

stephenmichaelf commented 4 years ago

@AnthonyDiSanti We had another discussion on this and we are OK making the change. Would you mind removing the _ at the start of the function since it won't be private?

Thanks for taking the time to provide thoughtful responses!

msftclas commented 4 years ago

CLA assistant check
All CLA requirements met.

AnthonyDiSanti commented 4 years ago

Thank you for reconsidering and approving this request @stephenmichaelf . I've renamed _processRequest to processRequest as per your guidance, squashed the commits, and rebased off the latest master. I've also confirmed that all tests pass locally for me and signed the CLA. Please let me know if there's anything else I can do to help. Thank you again!