Some of the code in httpserver.cpp was unreachable, and didn't follow the developer notes. Continuation of #26570
refactor: Deleted unreachable code in httpserver.cpp #26666
pull JoaoAJMatos wants to merge 2 commits into bitcoin:master from JoaoAJMatos:master changing 1 files +3 −11-
JoaoAJMatos commented at 5:44 PM on December 8, 2022: contributor
-
DrahtBot commented at 5:44 PM on December 8, 2022: contributor
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
Reviews
See the guideline for information on the review process.
Type Reviewers Stale ACK stickies-v If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
- DrahtBot added the label Refactoring on Dec 8, 2022
- stickies-v approved
-
stickies-v commented at 11:44 AM on December 9, 2022: contributor
Thank you for picking this up again @JoaoAJMatos - could you please update the PR description to reflect that this is a continuation of #26570 so people can see the discussion that happened there?
ACK c695dd91e - I checked that all the suggestions made in #26570 are implemented here. It removes unreachable code and improves adherence to developer notes, without behaviour change.
nit: I would suggest updating the current commit messages to prefix them with "refactor: ". See e.g. this guide on how you can do that if you're unsure.
If I may offer some further suggestions to your contribution workflow:
- it would have been more productive to not close the original PR but just clean up the existing branch and force push it instead of opening a new PR, so the entire discussion is all in one place. If you're not familiar with git rebasing and force pushing, I'd warmly recommend doing some reading/tutorials on that as it's an essential skill (and not too long to learn) for contributing to projects like Bitcoin Core. Also best to practice these operations on a private repo (e.g. your clone https://github.com/JoaoAJMatos/bitcoin) first instead of this PR to minimize unnecessary updates that require review time. See e.g. this guide as well: https://obc.256k1.dev/#_development_workflow
- I would work on a feature branch instead of working on
master(git checkout -b httpserver-delete-unreachable-codefor example) so you can easily switch between different PRs and also have a view of the currentmaster
-
7fd3b9491b
refactor: Deleted unreachable code in httpserver.cpp
Removed all break statements from both RequestMethodString and GetRequestMethod functions as they were unreachable
-
8f5c560e11
refactor: Refactored RequestMethodString function to follow developer notes
Removed the default case in the switch statement in order to comply with the Developer Notes
- JoaoAJMatos force-pushed on Dec 9, 2022
- JoaoAJMatos requested review from stickies-v on Dec 10, 2022
- stickies-v approved
-
stickies-v commented at 11:55 AM on December 10, 2022: contributor
re-ACK 8f5c560
- maflcko merged this on Dec 10, 2022
- maflcko closed this on Dec 10, 2022
- sidhujag referenced this in commit de862d954e on Dec 10, 2022
- bitcoin locked this on Dec 10, 2023
Contributors
Labels