Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Add helper methods to request class#97

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
jthomerson wants to merge1 commit intojeremydaly:main
base:main
Choose a base branch
Loading
fromjthomerson:add-request-helper-methods

Conversation

@jthomerson
Copy link
Contributor

No description provided.

@jthomerson
Copy link
ContributorAuthor

@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 forBuild 224

  • 0 of4(0.0%) changed or added relevant lines in1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.09%) to98.592%

Changes Missing CoverageCovered LinesChanged/Added Lines%
lib/request.js040.0%
TotalsCoverage Status
Change from baseBuild 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 theasArray functionality. The only reason I added it for some of theResponse 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, likegetHeaderAsArray(), 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 thegetQuery() seems like it might make sense. But I don't think we'd want to add things likegetVersion() andgetRoute() as we are simply exposing these as properties. That seems like overkill.

I'm open to suggestions though.

@jthomerson
Copy link
ContributorAuthor

Thanks for the quick response. Yeah, I wasn't sure about theasArray 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 acrude 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 frommaster tomainMarch 24, 2021 15:04
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jthomerson@coveralls@jeremydaly

[8]ページ先頭

©2009-2025 Movatter.jp