Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add helper methods to request class #97

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jthomerson
Copy link
Contributor

No description provided.

@jthomerson
Copy link
Contributor Author

@jeremydaly I wasn't sure the best way to add tests for these methods due to #96. Also, I wasn't sure if you'd want other helper methods. I'm going to integrate this branch into my app and test it there (so consider this a WIP). Please provide feedback on naming / other helper methods / test methodology and I can update this PR.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 224

  • 0 of 4 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.09%) to 98.592%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/request.js 0 4 0.0%
Totals Coverage Status
Change from base Build 221: -1.09%
Covered Lines: 584
Relevant Lines: 589

💛 - Coveralls

@jeremydaly
Copy link
Owner

Thanks for this @jthomerson. Naming conventions seem fine, but I still wavering on the asArray functionality. The only reason I added it for some of the Response methods was for backwards compatibility, but I'm wondering if new features should standardize on returning arrays by default. My other thought was to add convenience methods, like getHeaderAsArray(), but I don't want to bloat the project. What are you thoughts?

I'm not sure how many of these we need, either. Like the getQuery() seems like it might make sense. But I don't think we'd want to add things like getVersion() and getRoute() as we are simply exposing these as properties. That seems like overkill.

I'm open to suggestions though.

@jthomerson
Copy link
Contributor Author

Thanks for the quick response. Yeah, I wasn't sure about the asArray part either, but added it to try to be consistent with the response methods. I definitely think that the most common usecase is to get a single value - not an array. So I wouldn't want to see arrays returned by default.

I think a separate function that returns arrays if you want it would be better than a flag because then you could actually take advantage of return types to know you're getting a string vs an array. (that thought just made me realize I forgot to update the t.js ... which leads me to this question: curious your reasoning on developing it in JS and exposing TS files vs just developing it in TS)

Somewhat separately: I had a crude bunch of utilities that we'd been using for all of our APIs, and I'm investigating getting rid of them to use only your Lambda API lib. Based on this first conversion, I'm going to have a laundry list of suggestions on the API, documentation, etc. What's your preferred way of getting those?

Base automatically changed from master to main March 24, 2021 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants