You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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
The new code raises ValueError exceptions, but it might be better to raise domain-specific exceptions (like IECLoginError) for consistency with the rest of the codebase.
ifnotauthorize_response.strip().startswith("<!DOCTYPE html>") andnotauthorize_response.strip().startswith("<html"):
raiseValueError("Response is not an HTML document")
# B) Use BeautifulSoup to extract the code valuesoup=BeautifulSoup(authorize_response, "html.parser")
code_input=soup.find("input", {"name": "code"})
ifnotcode_input:
raiseValueError("Code input not found in HTML")
The code uses the default HTML parser without specifying a parser type explicitly. Consider specifying a parser like 'html.parser' or 'lxml' for consistent behavior across environments.
The action failed during the linting process. The linter found an undefined name error in iec_api/login.py at line 25. The code is trying to use random but it's not imported or defined. The specific error is: F821 Undefined name random.
Relevant error logs:
1: ##[group]Runner Image Provisioner2: Hosted Compute Agent
...
375: �[36;1mmake docker/lint�[0m376: shell: /usr/bin/bash -e {0}377: ##[endgroup]378: docker compose run "iec-api" poetry run ruff check .379: time="2025-06-06T06:56:27Z" level=warning msg="/home/runner/work/py-iec-api/py-iec-api/docker-compose.yml: the attribute `version` is obsolete, it will be ignored, please remove it to avoid potential confusion"380: Network py-iec-api_default Creating381: Network py-iec-api_default Created382: iec_api/login.py:25:17: F821 Undefined name `random`383: |384: 23 | APP_REDIRECT_URI = "com.iecrn:/"385: 24 | code_verifier, code_challenge = pkce.generate_pkce_pair()386: 25 | STATE = "".join(random.choice(string.digits + string.ascii_letters) for _ in range(12))387: | ^^^^^^ F821388: 26 | IEC_OKTA_BASE_URL = "https://iec-ext.okta.com"389: |390: Found 1 error.391: make: *** [Makefile:41: docker/lint] Error 1392: ##[error]Process completed with exit code 2.393: Post job cleanup.
The params.response.text() is a coroutine that consumes the response body, which can only be read once. Calling it here will consume the response, making it unavailable for subsequent processing. This could cause errors when the actual handler tries to read the response content later.
Why: This is a critical issue as calling await params.response.text() in a debug handler consumes the response body, making it unavailable for subsequent processing. This could break application functionality.
Medium
Maintain field consistency
The code field is being changed from a required field with no default value to an optional field with a default value of None. This might cause issues if existing code relies on this field always having a value. Consider keeping it required or adding validation logic if a non-None value is expected in certain contexts.
Why: Valid concern about changing from required to optional field, but the improved_code using field(default=None) is functionally equivalent to = None and doesn't address the underlying compatibility concern.
Low
Fix typo in error message
There's a typo in the error message "Autorize" instead of "Authorize". Fix the spelling to ensure consistent and professional error messages.
# A) Validate that the response is indeed an HTML
if not authorize_response.strip().startswith("<!DOCTYPE html>") and not authorize_response.strip().startswith(
"<html"
):
- raise IECLoginError(-1, "Autorize Response is not an HTML document")+ raise IECLoginError(-1, "Authorize Response is not an HTML document")
Apply / Chat
Suggestion importance[1-10]: 4
__
Why: Minor typo fix changing "Autorize" to "Authorize" improves code professionalism but has minimal functional impact.
Add a null check for the extracted code value. If the input element exists but has no value attribute, this could return None and cause issues downstream.
code = code_input.get("value")
+if not code:+ raise IECLoginError("Code value is empty or missing")
Suggestion importance[1-10]: 6
__
Why: Adding a null check for the code value is good defensive programming that prevents potential issues if the HTML input element has no value attribute. This improves robustness.
Low
General
Use specific error type
Replace the generic ValueError with the more specific IECLoginError that's already imported in this module. This ensures consistent error handling throughout the application and provides better context for debugging.
# A) Validate that the response is indeed an HTML
if not authorize_response.strip().startswith("<!DOCTYPE html>") and not authorize_response.strip().startswith("<html"):
- raise ValueError("Response is not an HTML document")+ raise IECLoginError("Response is not an HTML document")
Suggestion importance[1-10]: 5
__
Why: Using IECLoginError instead of ValueError provides better error handling consistency and more specific context for debugging. This is a minor but worthwhile improvement.
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.
GuyKh
force-pushed
the
htmlParsing
branch
3 times, most recently
from
PR Type
enhancement
Description
Switch authorization HTML parsing to BeautifulSoup4 for robustness
Add BeautifulSoup4 as a new project dependency
Improve error handling for non-HTML and missing code input
Set default value for
codeinResponseDescriptorChanges walkthrough 📝
login.py
Switch to BeautifulSoup4 for HTML parsing and add error checksiec_api/login.py
code
response_descriptor.py
Set default value for optional code fieldiec_api/models/response_descriptor.py
codeto None inResponseDescriptorpyproject.toml
Add BeautifulSoup4 to project dependenciespyproject.toml