refactor: get_working_proxy#216
Merged
Merged
Conversation
Contributor
审阅者指南此 PR 通过引入类型注解、通过 重构后代理连接检查的序列图sequenceDiagram
participant get_working_proxy
participant ThreadPoolExecutor
participant check_proxy
participant can_connect
participant requests
get_working_proxy->>ThreadPoolExecutor: Submit check_proxy for each proxy
ThreadPoolExecutor->>check_proxy: Execute check_proxy(proxy)
check_proxy->>can_connect: Call can_connect(url)
can_connect->>requests: requests.head(url, timeout)
requests-->>can_connect: Response (ok or exception)
can_connect-->>check_proxy: Return True/False
check_proxy-->>ThreadPoolExecutor: Return (url, result)
ThreadPoolExecutor-->>get_working_proxy: Return first successful proxy or ""
文件级更改
提示和命令与 Sourcery 交互
自定义您的体验访问您的 仪表盘 以：
获取帮助Original review guide in EnglishReviewer's GuideThe PR refactors the proxy connectivity flows by introducing type annotations, streamlining connectivity checks via requests.head(...).ok, unifying check_proxy to return a (url,success) tuple, and simplifying the get_working_proxy concurrency logic with proper executor shutdown and an empty-string fallback. Sequence diagram for the refactored proxy connectivity checksequenceDiagram
participant get_working_proxy
participant ThreadPoolExecutor
participant check_proxy
participant can_connect
participant requests
get_working_proxy->>ThreadPoolExecutor: Submit check_proxy for each proxy
ThreadPoolExecutor->>check_proxy: Execute check_proxy(proxy)
check_proxy->>can_connect: Call can_connect(url)
can_connect->>requests: requests.head(url, timeout)
requests-->>can_connect: Response (ok or exception)
can_connect-->>check_proxy: Return True/False
check_proxy-->>ThreadPoolExecutor: Return (url, result)
ThreadPoolExecutor-->>get_working_proxy: Return first successful proxy or ""
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
嘿 @LY-Xiang - 我已经审阅了你的更改，它们看起来很棒！
AI 代理提示
请解决此代码审查中的评论：
## 个人评论
### 评论 1
<location> `install_windows.py:472` </location>
<code_context>
- if result is not None:
- return result
- return None
+ executor = ThreadPoolExecutor()
+ try:
+ for future in as_completed(
+ [executor.submit(check_proxy, proxy) for proxy in proxies]
+ ):
+ proxy, result = future.result()
+ if result:
+ return proxy
+ return ""
+ finally:
+ executor.shutdown(wait=False, cancel_futures=True)
</code_context>
<issue_to_address>
ThreadPoolExecutor 未受代理数量限制，这可能会影响资源使用。
明确将 max_workers 设置为 len(proxies) 可能会提供更可预测的资源使用和并行性，尤其是在代理数量变化时。
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
executor = ThreadPoolExecutor()
=======
executor = ThreadPoolExecutor(max_workers=len(proxies))
>>>>>>> REPLACE
</suggested_fix>
### 评论 2
<location> `install_windows.py:482` </location>
<code_context>
+ return proxy
+ return ""
+ finally:
+ executor.shutdown(wait=False, cancel_futures=True)
</code_context>
<issue_to_address>
如果所有 future 都已完成，则使用 cancel_futures=True 可能不是必需的。
考虑删除 cancel_futures=True，除非您预期有未完成的 future，因为 as_completed 确保所有 future 都已完成。
</issue_to_address>
### 评论 3
<location> `install_windows.py:463` </location>
<code_context>
- except requests.exceptions.RequestException:
- pass
- return None
+def check_proxy(url: str, timeout: float = 5) -> tuple[str, bool]:
+ return url, can_connect(
+ f"{url}/https://github.com/Mzdyl/LiteLoaderQQNT_Install/archive/refs/heads/main.zip",
</code_context>
<issue_to_address>
考虑重构 check_proxy 以仅返回布尔值，并在 get_working_proxy 中使用 ThreadPoolExecutor 和上下文管理器以及 executor.map，以实现更简洁和可维护的实现。
您可以通过以下方式简化这两个函数：
1. 使 `check_proxy` 仅返回 `bool` 值
2. 使用 `with ThreadPoolExecutor(...)` 上下文管理器 + `executor.map`
3. 使用单个 `next(...)` 选择第一个可用的代理
这恢复了简洁的流程，并避免了调用方中的手动关闭或元组解包：
```python
def check_proxy(proxy: str, timeout: float = 5) -> bool:
test_url = f"{proxy}/https://github.com/Mzdyl/LiteLoaderQQNT_Install/archive/refs/heads/main.zip"
return can_connect(test_url, timeout)
```
```python
from concurrent.futures import ThreadPoolExecutor
def get_working_proxy() -> str:
proxies = get_github_proxy_urls()
# use len(proxies) to maximize concurrency, and auto‐shutdown
with ThreadPoolExecutor(max_workers=len(proxies)) as executor:
# map each proxy to (proxy, ok)
results = executor.map(lambda p: (p, check_proxy(p)), proxies)
# return the first proxy where ok is True, or empty string
return next((p for p, ok in results if ok), "")
```
这保持了相同的行为，恢复了上下文管理器，并删除了样板解包和手动执行器关闭。
</issue_to_address>帮助我更有用！请点击每个评论上的 👍 或 👎，我将使用反馈来改进您的评论。
Original comment in English
Hey @LY-Xiang - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `install_windows.py:472` </location>
<code_context>
- if result is not None:
- return result
- return None
+ executor = ThreadPoolExecutor()
+ try:
+ for future in as_completed(
+ [executor.submit(check_proxy, proxy) for proxy in proxies]
+ ):
+ proxy, result = future.result()
+ if result:
+ return proxy
+ return ""
+ finally:
+ executor.shutdown(wait=False, cancel_futures=True)
</code_context>
<issue_to_address>
ThreadPoolExecutor is not limited by the number of proxies, which may impact resource usage.
Explicitly setting max_workers to len(proxies) may provide more predictable resource usage and parallelism, especially when the number of proxies varies.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
executor = ThreadPoolExecutor()
=======
executor = ThreadPoolExecutor(max_workers=len(proxies))
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `install_windows.py:482` </location>
<code_context>
+ return proxy
+ return ""
+ finally:
+ executor.shutdown(wait=False, cancel_futures=True)
</code_context>
<issue_to_address>
Using cancel_futures=True may not be necessary if all futures have completed.
Consider removing cancel_futures=True unless you anticipate incomplete futures, as as_completed ensures all are finished.
</issue_to_address>
### Comment 3
<location> `install_windows.py:463` </location>
<code_context>
- except requests.exceptions.RequestException:
- pass
- return None
+def check_proxy(url: str, timeout: float = 5) -> tuple[str, bool]:
+ return url, can_connect(
+ f"{url}/https://github.com/Mzdyl/LiteLoaderQQNT_Install/archive/refs/heads/main.zip",
</code_context>
<issue_to_address>
Consider refactoring check_proxy to return only a bool and using ThreadPoolExecutor with a context manager and executor.map in get_working_proxy for a more concise and maintainable implementation.
You can simplify both functions by
1. Making `check_proxy` return only a `bool`
2. Using a `with ThreadPoolExecutor(...)` context manager + `executor.map`
3. Picking the first working proxy with a single `next(...)`
This restores the concise flow and avoids manual shutdown or tuple‐unpacking in the caller:
```python
def check_proxy(proxy: str, timeout: float = 5) -> bool:
test_url = f"{proxy}/https://github.com/Mzdyl/LiteLoaderQQNT_Install/archive/refs/heads/main.zip"
return can_connect(test_url, timeout)
```
```python
from concurrent.futures import ThreadPoolExecutor
def get_working_proxy() -> str:
proxies = get_github_proxy_urls()
# use len(proxies) to maximize concurrency, and auto‐shutdown
with ThreadPoolExecutor(max_workers=len(proxies)) as executor:
# map each proxy to (proxy, ok)
results = executor.map(lambda p: (p, check_proxy(p)), proxies)
# return the first proxy where ok is True, or empty string
return next((p for p, ok in results if ok), "")
```
This keeps the same behavior, restores the context manager, and removes boilerplate unpacking and manual executor shutdown.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sourcery 总结
重构了用于检查代理连接性的辅助函数，改进了类型提示，精简了逻辑，并实现了更安全的执行器关闭。
增强功能：
can_connect中添加类型注解并统一异常处理can_connect以使用response.okcheck_proxy以返回 URL 和针对 ZIP 归档文件的连接结果的元组get_working_proxy以内联方式提交 futures，正确关闭执行器，并在没有可用代理时返回空字符串Original summary in English
Summary by Sourcery
Refactor helper functions for checking proxy connectivity with improved type hints, streamlined logic, and safer executor shutdown.
Enhancements: