chore: Simplify if expression by using or
#659
Conversation
There was a problem hiding this comment.
Hey, @yezz123! I've got a few comments on this change.
This PR title doesn't correspond to its contents. Could you update it to represent the effect of applying this patch to the develop branch? Also, it'd be useful to have the PR description filled out explaining the motivation behind this change.
Since there seem to be a number of refactoring pieces, it's probably best to add inline comments to each of these places in the Files tab explaining why you think this is needed.
I've also submitted a number of PRs setting up linting improvements in the CI, may I ask you to wait until @abhinavsingh merges them and then rebase this on top so that those checkers are taken into account?
P.S. FYI here's a nice article with pointers on how to compose great pull requests: https://mtlynch.io/code-review-love/.
Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
|
@yezz123 This PR has again run into "This branch is out-of-date with the base branch". Unsure why am I unable to "Update Branch" from within PR. Could be because of permission issues on upstream fork? Can you take a look. Thank you
|
Codecov Report
@@ Coverage Diff @@
## develop #659 +/- ##
===========================================
+ Coverage 74.01% 74.15% +0.13%
===========================================
Files 84 84
Lines 3264 3254 -10
===========================================
- Hits 2416 2413 -3
+ Misses 848 841 -7
Continue to review full report at Codecov.
|
|
@abhinavsingh could you check now? |
| @@ -92,10 +89,6 @@ def handle_client_request( | |||
| headers={b'Connection': b'close'}, | |||
| reason=b'Blocked', | |||
| ) | |||
| # stop looping through filter list | |||
| break | |||
There was a problem hiding this comment.
This removes unreachable code. Could you also try dropping the linter exclusions at https://github.com/abhinavsingh/proxy.py/blob/7c5877b/.pylintrc#L129 and https://github.com/abhinavsingh/proxy.py/blob/7c5877b/.flake8#L149 to see if this was the only place with this type of a linting violation?
Can you please update again so that we can get your PR in. Leave a TODO for linter checks as proposed by @webknjaz and we can revisit it outside of this PR. Wdyt folks? |
I guess adding two lines with todo from the linter configs does not differ much from removing two lines. If the violations are fixed, why keep the ignores? |
Agreed, if PR author has know-how about underlying workflows (not GHA but lint related), best to give it a try within the PR. Having said that, I don't expect contributors to necessarily know where to tweak lint warnings or improve them. A regular contributor probably should, but not necessary for first-time contributors. TL;DR -- As a first time contributor if CI passes, task is done within that PR context. Any other enhancements can be done in follow-up PRs. Wdyt? I am also considering writing a contributors guide. We can state various pre-requisites for first-time and regular contributors. Currently, I see first time contributors struggling with a green CI (multiple back-and-forth). |
I guess that's fine. OTOH if the contributor is able to remove a specific line in a specific file, why not ask about it? I've linked the exact lines @ #659 (comment). It's not that hard. If the contributor doesn't want to do this, they should state this explicitly, and this would be fine as well, I think. |
I agree about the comm part |
|
Several of these recommendations made into the code as part of refactoring. I have picked up on remaining changes including lint warning removal in #904 |
* Add `ProgramNamePlugin` * Update readme * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Remove `_compat.py` * Add suggestions coming from #659 Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* Merge pull request #896 from abhinavsingh/missed-scenarios Avoid registering invalid FD with selectors * [ProxyPool] Add support for basic authorization with upstream proxies (#897) * Use `Url` class to parse proxy pool entries * Add support for parsing user:pass from raw url bytes * Add `httpHeaders.PROXY_AUTHORIZATION` headers for upstream proxies * Add support for httpHeaders enum * Send base64 encoded proxy authorization header to upstream proxies * mypy fixes * Document proxy pool authentication support usage info * Add `conn_close` kwarg to packet builder utilities (#898) * Add `conn_close` kwarg to packet builder utilities, passing True will automatically add `Connection: close` header * Add `conn_close` to `HttpRequestRejected` responses * Raise `HttpProtocolException` instead of `ValueError` (#899) * Raise `HttpProtocolException` instead of `ValueError` for clean teardown of the offending connection * Fix circular import errors * Fix tests * fix lint errors * Avoid containerizing until check has passed * Ensure message for every `HttpProtocolException` raised (#900) * Response Packet Utilities (#903) * Add response pkt utility * Unused import * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix tests as some content is now by default gzipped based upon min compression config * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Remove unused * Update necessary tests to use `okResponse` utility * Add option to explicitly disable compression Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * Introduce `ProgramNamePlugin` plugin (#904) * Add `ProgramNamePlugin` * Update readme * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Remove `_compat.py` * Add suggestions coming from #659 Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * Update defaults for `--hostname` and `--local-executor` (#905) * do it * Change defaults * integrate with ci/cd updates * Fix ci * Add dockerfile `SKIP_OPENSSL` option which will allow us to build container without openssl * Skip openssl for latest tag, add another openssl tag for images with openssl support * Push separate openssl image to GHCR for every PR * `Work` can also be `TcpServerConnection`, not just `TcpClientConnection` (#906) * Work can also be TcpServerConnection, not just TcpClientConnection. More over, it can be any generic work type * Add py_class_role and py_obj * Port internal integration tests into public repo * Fix proxy py addr * Use cross-platform compat shasum * Change `--local-executor` flag semantics (#907) * Convert `--local-executor` in an integer flag, defaults to 1 i.e. enabled, use 0 to disable * Consider any value other than 1 as remote mode * Use integer to disable local executor mode in integration tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix mypy errors * Update flags in readme * Check for type * Remove pid file check for now * Update `--local-executor` flag usage Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * Clean shutdown on `SIGINT`, `SIGHUP`, `SIGTERM`, `SIGQUIT` (#908) * sys.exit on SIGINT, SIGHUP, SIGTERM * Add todo for pending signal actions * SIGHUP is win only * Remove frametype signature as it causes lint issues and we are not using it anyways * SIGQUIT is not on Win * Fix `HttpWebServerPacFilePlugin` broken routes logic (#915) * Fix `HttpWebServerPacFilePlugin` broken routes logic * lint * Proxy Auto-Configuration (PAC) file should not be compressed (#916) * Move `UpstreamConnectionPool` lifecycle within `Threadless` (#917) * Tie connection pool into Threadless * Pass upstream conn pool reference to work instances * Mark upstream conn pool as optional * spellcheck * Fix unused import * Define work lifecycle events for pool (#918) * Define work lifecycle events for pool * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Use isinstance * Use mocker fixture to pass CI on 3.6 and 3.7 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * Hook `UpstreamConnectionPool` lifecycle within `Threadless` (#921) * Hook connection pool lifecycle within threadless * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix test * Fix spell Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * pip prod(deps): bump sphinx from 4.3.1 to 4.3.2 (#902) Bumps [sphinx](https://github.com/sphinx-doc/sphinx) from 4.3.1 to 4.3.2. - [Release notes](https://github.com/sphinx-doc/sphinx/releases) - [Changelog](https://github.com/sphinx-doc/sphinx/blob/4.x/CHANGES) - [Commits](sphinx-doc/sphinx@v4.3.1...v4.3.2) --- updated-dependencies: - dependency-name: sphinx dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Abhinav Singh <126065+abhinavsingh@users.noreply.github.com> * pip prod(deps): bump paramiko from 2.8.1 to 2.9.1 (#923) * Optimize how `HttpProtocolHandler` delegates to the core plugins (#925) * Add `protocols` abstract static method to `HttpProtocolHandlerBase` which defines which HTTP specification is followed by the core plugin * lint * Fix tests * Lint fixes * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * [TlsParser] Refactored implementation from #748 (#922) * Refactored TlsParser based upon work done in #748 * Add missing `tls_server_hello.data`, thanks to @JerryKwan * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Pass `check.py` * Run check.py locally * Fix lint errors * Fix indentation issue * Ignore linkcheck for cloudflare links, GHA is getting a 403 reply, while the link actually works * Fix lint * codespell skip Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * pip prod(deps): bump tox from 3.24.4 to 3.24.5 (#924) Bumps [tox](https://github.com/tox-dev/tox) from 3.24.4 to 3.24.5. - [Release notes](https://github.com/tox-dev/tox/releases) - [Changelog](https://github.com/tox-dev/tox/blob/master/docs/changelog.rst) - [Commits](tox-dev/tox@3.24.4...3.24.5) --- updated-dependencies: - dependency-name: tox dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Abhinav Singh <126065+abhinavsingh@users.noreply.github.com> * pip prod(deps): bump twine from 3.7.0 to 3.7.1 (#927) Bumps [twine](https://github.com/pypa/twine) from 3.7.0 to 3.7.1. - [Release notes](https://github.com/pypa/twine/releases) - [Changelog](https://github.com/pypa/twine/blob/main/docs/changelog.rst) - [Commits](pypa/twine@3.7.0...3.7.1) --- updated-dependencies: - dependency-name: twine dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Abhinav Singh <126065+abhinavsingh@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
No description provided.