Skip to content

Consolidate fast_write calls in Server, extract early_hints assembly #2405

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.

Sign up for GitHub

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

Jump to bottom
Merged
nateberkopec merged 1 commit into from
Oct 5, 2020
Merged

Consolidate fast_write calls in Server, extract early_hints assembly #2405

nateberkopec merged 1 commit into from
Oct 5, 2020

Conversation

@MSP-Greg
Copy link
Member

@MSP-Greg MSP-Greg commented Oct 2, 2020

Description

Server#handle_request is a very long method, and also has many #fast_write calls. #fast_write calls, which involve rescue clauses, when done with a reasonably defined string in terms of length, should be minimized.

Also, extract code to assemble the 'early hints' string into #str_early_hints.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] or [ci skip] to the pull request title.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@nateberkopec nateberkopec added maintenance waiting-for-review Waiting on review from anyone labels Oct 2, 2020
@MSP-Greg
Copy link
Member Author

MSP-Greg commented Oct 3, 2020 •
edited
Loading

I named the method containing the extracted code str_early_hints. Wondering about something like assemble_response_early_hints or resp_early_hints, etc? Reason for asking is is I'd like to extract code for the headers, and the method would be named similarly.

Two items: a) intent on extracting/refactoring without moving exception handling code, and b) get some type of coverage working before adding/changing tests to specifically run against the new methods. Once things are extracted, there will be less of a need to use sockets to test the particulars of the code.

Re coverage, I need to investigate more, but it looks like Coveralls and Codecov support GH Actions. Not sure about CodeClimate...

@nateberkopec nateberkopec merged commit b089768 into puma:master Oct 5, 2020
@MSP-Greg MSP-Greg deleted the str-early-hints branch October 5, 2020 13:44
@MSP-Greg MSP-Greg removed the waiting-for-review Waiting on review from anyone label Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

maintenance

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MSP-Greg @nateberkopec