Feat/groh/add generic http grpc header #109
Conversation
WalkthroughThe pull request introduces support for custom HTTP headers in scanning functionality. Users can now specify headers via a new CLI argument ( Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as Command Line Interface
participant PR as process_req_headers
participant S as Scanner
participant API as ScanossApi
participant GRPC as ScanossGrpc
U->>CLI: Run scan command with -hdr "Key: Value"
CLI->>PR: Parse headers input
PR-->>CLI: Return headers dictionary
CLI->>S: Initialize Scanner(req_headers)
S->>API: Pass req_headers to API
S->>GRPC: Pass req_headers to GRPC
API->>API: load_generic_headers processing
GRPC->>GRPC: load_generic_headers processing
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
tests/grpc-client-test.py (1)
124-131: Good test for verifying header handlingThe test correctly verifies that custom headers are properly added to the gRPC client's metadata, along with standard headers. This ensures the header handling functionality works as expected.
Fix the line length issue by splitting the initialization:
- grpc_client = ScanossGrpc(debug=True, req_headers={'x-api-key': '123455', 'generic-header': 'generic-header-value'}) + req_headers = { + 'x-api-key': '123455', + 'generic-header': 'generic-header-value' + } + grpc_client = ScanossGrpc(debug=True, req_headers=req_headers)🧰 Tools
🪛 Ruff (0.8.2)
125-125: Line too long (124 > 120)
(E501)
tests/scanossapi-test.py (2)
30-37: Enhance test assertion to verify header valuesThe current test checks that there are no unexpected keys in the headers, but it doesn't verify that expected keys are present or that values are correct. This could allow bugs where expected headers are missing or have incorrect values.
Consider enhancing the test with more precise assertions:
- required_keys = ('x-api-key', 'User-Agent', 'user-agent', 'generic-header') - valid_headers = True - for key, value in scanoss_api.headers.items(): - if key not in required_keys: - valid_headers = False - self.assertTrue(valid_headers) + # Expected headers with values + expected_headers = { + 'x-api-key': '123455', + 'User-Agent': f'scanoss-py/{__version__}', + 'user-agent': f'scanoss-py/{__version__}', + 'generic-header': 'generic-header-value' + } + + # Verify that expected headers are present with correct values + for key, expected_value in expected_headers.items(): + self.assertIn(key, scanoss_api.headers, f"Header '{key}' is missing") + self.assertEqual(scanoss_api.headers[key], expected_value, + f"Header '{key}' has incorrect value") + + # Verify no unexpected headers + for key in scanoss_api.headers: + self.assertIn(key, expected_headers, f"Unexpected header '{key}' found")🧰 Tools
🪛 Ruff (0.8.2)
31-31: Line too long (123 > 120)
(E501)
31-31: Line exceeds maximum lengthThe line length (123 characters) exceeds the maximum allowed length of 120 characters.
- scanoss_api = ScanossApi(debug=True, req_headers={'x-api-key': '123455', 'generic-header': 'generic-header-value'}) + scanoss_api = ScanossApi( + debug=True, + req_headers={'x-api-key': '123455', 'generic-header': 'generic-header-value'} + )🧰 Tools
🪛 Ruff (0.8.2)
31-31: Line too long (123 > 120)
(E501)
tests/cli-test.py (1)
51-52: Line exceeds maximum lengthThe line length (121 characters) exceeds the maximum allowed length of 120 characters.
- self.assertEqual(processed_headers, expected_headers, - f"Headers don't match expected values.\nGot: {processed_headers}\nExpected: {expected_headers}") + self.assertEqual(processed_headers, expected_headers, + f"Headers don't match expected values.\n" + f"Got: {processed_headers}\n" + f"Expected: {expected_headers}")🧰 Tools
🪛 Ruff (0.8.2)
52-52: Line too long (121 > 120)
(E501)
src/scanoss/cli.py (2)
28-29: Remove unused importsThe imports
arrayandparse_headerare not used in the code.-from array import array -from cgi import parse_header🧰 Tools
🪛 Ruff (0.8.2)
28-28:
array.arrayimported but unusedRemove unused import:
array.array(F401)
29-29:
cgi.parse_headerimported but unusedRemove unused import:
cgi.parse_header(F401)
1476-1499: Well-implemented header processing functionThe
process_req_headersfunction correctly handles empty input, header parsing, and whitespace trimming. It also ignores malformed headers missing a colon separator.Consider defining a constant for the expected number of parts after splitting:
+ # Constants + HEADER_KEY_VALUE_PARTS = 2 # Check if headers_array is empty if not headers_array: # Array is empty return {} dict_headers = {} for header_str in headers_array: # Split each "Name: Value" header parts = header_str.split(":", 1) - if len(parts) == 2: + if len(parts) == HEADER_KEY_VALUE_PARTS: name = parts[0].strip() value = parts[1].strip() dict_headers[name] = value🧰 Tools
🪛 Ruff (0.8.2)
1495-1495: Magic value used in comparison, consider replacing
2with a constant variable(PLR2004)
src/scanoss/scanossapi.py (2)
95-95: Magic value in timeout validationLine 95 uses a magic value
5for minimum timeout, which could be replaced with a constant for better readability.+ MIN_TIMEOUT = 5 - self.timeout = timeout if timeout > 5 else 180 + self.timeout = timeout if timeout > MIN_TIMEOUT else 180🧰 Tools
🪛 Ruff (0.8.2)
95-95: Magic value used in comparison, consider replacing
5with a constant variable(PLR2004)
282-282: Environment variable check mismatchThe method checks for
SCANOSS_GRPC_URLbut elsewhere in the code,SCANOSS_SCAN_URLis used for the REST API URL.- if not self.url and not os.environ.get('SCANOSS_GRPC_URL'): + if not self.url and not os.environ.get('SCANOSS_SCAN_URL'):src/scanoss/scanossgrpc.py (2)
115-120: Consider refactoring the URL initialization logic.The URL initialization logic is now split across multiple places, making it harder to follow. Consider consolidating this into a single block to improve readability and maintenance.
- self.url = url if url else SCANOSS_GRPC_URL - if self.api_key and not url and not os.environ.get('SCANOSS_GRPC_URL'): - self.url = DEFAULT_URL2 # API key specific and no alternative URL, so use the default premium - self.url = self.url.lower() - self.orig_url = self.url # Used for proxy lookup + # Set URL based on configuration priority + if url: + self.url = url + elif self.api_key and not os.environ.get('SCANOSS_GRPC_URL'): + self.url = DEFAULT_URL2 # API key specific and no alternative URL, so use the default premium + else: + self.url = SCANOSS_GRPC_URL + + self.url = self.url.lower() + self.orig_url = self.url # Used for proxy lookup
482-496: Good implementation of generic headers loading with minor improvement opportunity.The
load_generic_headersmethod correctly adds custom headers to metadata and handles the special case for thex-api-keyheader. However, there's duplication with a similar method inscanossapi.py.Consider refactoring this method to avoid duplicating logic between
ScanossGrpcandScanossApiclasses. You could:
- Extract this to a common utility function
- Move this to the parent class
ScanossBaseand make it accept a parameter to specify whether to updateheadersormetadata- Use a consistent approach for header storage between both classes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CLIENT_HELP.md(2 hunks)src/scanoss/cli.py(12 hunks)src/scanoss/components.py(2 hunks)src/scanoss/scanner.py(4 hunks)src/scanoss/scanossapi.py(3 hunks)src/scanoss/scanossgrpc.py(4 hunks)tests/cli-test.py(1 hunks)tests/grpc-client-test.py(1 hunks)tests/scanossapi-test.py(1 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
tests/grpc-client-test.py (1)
src/scanoss/scanossgrpc.py (1) (1)
ScanossGrpc(63:495)
tests/scanossapi-test.py (2)
src/scanoss/scanossapi.py (1) (1)
ScanossApi(49:285)tests/cli-test.py (1) (1)
MyTestCase(28:60)
src/scanoss/scanossapi.py (1)
src/scanoss/scanossgrpc.py (1) (1)
load_generic_headers(482:495)
src/scanoss/scanossgrpc.py (1)
src/scanoss/scanossapi.py (1) (1)
load_generic_headers(272:285)
🪛 Ruff (0.8.2)
tests/cli-test.py
52-52: Line too long (121 > 120)
(E501)
tests/grpc-client-test.py
125-125: Line too long (124 > 120)
(E501)
tests/scanossapi-test.py
31-31: Line too long (123 > 120)
(E501)
src/scanoss/scanossapi.py
95-95: Magic value used in comparison, consider replacing 5 with a constant variable
(PLR2004)
src/scanoss/cli.py
28-28: array.array imported but unused
Remove unused import: array.array
(F401)
29-29: cgi.parse_header imported but unused
Remove unused import: cgi.parse_header
(F401)
1495-1495: Magic value used in comparison, consider replacing 2 with a constant variable
(PLR2004)
🔇 Additional comments (12)
CLIENT_HELP.md (1)
225-265: Clear and comprehensive documentation for the new header functionalityThe documentation clearly explains how to use the new
-hdroption to include custom headers in both scan and component commands. The examples are well-structured and cover common use cases.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
249-249: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
250-250: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
251-251: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
252-252: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
253-253: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
src/scanoss/components.py (2)
54-54: LGTM - Good parameter definitionThe addition of the
req_headersparameter with proper type hinting and default value is well-implemented.
84-84: LGTM - Proper parameter propagationThe request headers are correctly passed to the ScanossGrpc constructor, ensuring that the headers are properly utilized in API calls.
src/scanoss/scanner.py (3)
109-109: LGTM - Good parameter definitionThe addition of the
req_headersparameter with proper type hinting and default value is well-implemented.
133-133: LGTM - Proper instance variable assignmentThe request headers are correctly stored as an instance variable, making them available throughout the class.
161-161: LGTM - Comprehensive header propagationThe request headers are properly passed to both the ScanossApi and ScanossGrpc constructors, ensuring consistent header handling across both REST and gRPC protocols.
Also applies to: 175-175
tests/cli-test.py (1)
30-60: Well-structured comprehensive testThis test effectively covers various header formats including normal cases, whitespace handling, and malformed input rejection. The assertions are thorough, checking both equality and specific key presence/values.
🧰 Tools
🪛 Ruff (0.8.2)
52-52: Line too long (121 > 120)
(E501)
src/scanoss/cli.py (2)
573-578: Good CLI argument implementationThe header argument implementation follows the CLI's established pattern and provides a clear help message with examples.
719-719: Header processing implementation is consistentThe
process_req_headersfunction is consistently used across all relevant components and functions that make API requests.Also applies to: 916-916, 1254-1254, 1289-1289, 1324-1324, 1362-1362, 1407-1407, 1432-1432
src/scanoss/scanossapi.py (1)
71-71: Good parameter addition and initializationThe
req_headersparameter is properly added with a default value ofNone, andself.req_headersis initialized with appropriate handling of theNonecase.Also applies to: 98-99
src/scanoss/scanossgrpc.py (2)
105-105: LGTM - Properly storing the headers parameter.The instance variable is correctly set from the parameter.
113-113: LGTM - Appropriate method call placement.The
load_generic_headers()method is called at the right time in the initialization sequence, after basic metadata has been set up.
| self.url = url | ||
| self.api_key = api_key |
There was a problem hiding this comment.
🛠️ Refactor suggestion
URL and API key initialization is redundant
There's redundancy in setting self.url and self.api_key - they're initialized twice, which could cause confusion. The initialization at lines 90-91 could be overwritten by the second initialization.
Consolidate the initialization logic to avoid redundancy:
- self.url = url
- self.api_key = api_key
self.sbom = None
self.scan_format = scan_format if scan_format else 'plain'
self.flags = flags
self.timeout = timeout if timeout > 5 else 180
self.retry_limit = retry if retry >= 0 else 5
self.ignore_cert_errors = ignore_cert_errors
self.req_headers = req_headers if req_headers else {}
self.headers = {}
if ver_details:
self.headers['x-scanoss-client'] = ver_details
if self.api_key:
self.headers['X-Session'] = self.api_key
self.headers['x-api-key'] = self.api_key
self.headers['User-Agent'] = f'scanoss-py/{__version__}'
self.headers['user-agent'] = f'scanoss-py/{__version__}'
self.load_generic_headers()
+ # Initialize URL and API key after loading generic headers to ensure proper precedence
self.url = url if url else SCANOSS_SCAN_URL
self.api_key = api_key if api_key else SCANOSS_API_KEY
if self.api_key and not url and not os.environ.get('SCANOSS_SCAN_URL'):
self.url = DEFAULT_URL2 # API key specific and no alternative URL, so use the default premiumAlso applies to: 108-114
agustingroh
force-pushed
the
feat/groh/add-generic-http-grpc-header
branch
from
d181b91 to
2541057
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/scanoss/scanossapi.py (1)
108-114:⚠️ Potential issueInitialization ordering issue with URL and API key
The current ordering of operations creates a logical issue:
load_generic_headers()at line 108 may setself.urlandself.api_key- These values are immediately overwritten by lines 110-113
This renders the conditional URL and API key assignment in
load_generic_headers()ineffective.Apply this change to fix the ordering issue:
- self.load_generic_headers() - - self.url = url if url else SCANOSS_SCAN_URL - self.api_key = api_key if api_key else SCANOSS_API_KEY - if self.api_key and not url and not os.environ.get('SCANOSS_SCAN_URL'): - self.url = DEFAULT_URL2 # API key specific and no alternative URL, so use the default premium + # Set URL and API key from parameters or environment + self.url = url if url else SCANOSS_SCAN_URL + self.api_key = api_key if api_key else SCANOSS_API_KEY + + # Load generic headers (which may update URL and API key from headers) + self.load_generic_headers() + + # Set premium URL if API key is present but no explicit URL was provided + if self.api_key and not url and not os.environ.get('SCANOSS_SCAN_URL'): + self.url = DEFAULT_URL2 # API key specific and no alternative URL, so use the default premium
🧹 Nitpick comments (3)
tests/grpc-client-test.py (1)
124-131: Add comprehensive metadata validation for GRPC headersThe test verifies that metadata keys are in the required set, but doesn't check if all required keys are present in the metadata or validate the header values.
def test_grpc_generic_metadata(self): - grpc_client = ScanossGrpc(debug=True, req_headers={'x-api-key': '123455', 'generic-header': 'generic-header-value'}) + api_key = '123455' + header_value = 'generic-header-value' + grpc_client = ScanossGrpc(debug=True, req_headers={'x-api-key': api_key, 'generic-header': header_value}) required_keys = ('x-api-key', 'user-agent', 'x-scanoss-client', 'generic-header') - valid_metadata = True + # Check all metadata keys are in required keys for key, value in grpc_client.metadata: - if key not in required_keys: - valid_metadata = False - self.assertTrue(valid_metadata) + self.assertIn(key, required_keys, f"Unexpected metadata key: {key}") + + # Check all required keys are in metadata + metadata_dict = dict(grpc_client.metadata) + for key in required_keys: + self.assertIn(key, metadata_dict, f"Required metadata key missing: {key}") + + # Verify specific values + self.assertEqual(dict(grpc_client.metadata).get('x-api-key'), api_key) + self.assertEqual(dict(grpc_client.metadata).get('generic-header'), header_value)🧰 Tools
🪛 Ruff (0.8.2)
125-125: Line too long (124 > 120)
(E501)
🪛 GitHub Actions: Lint
[error] 125-125: Line too long (124 > 120).
tests/scanossapi-test.py (1)
30-37: Improve header validation test with more comprehensive assertionsSimilar to the GRPC test, this test only checks if all headers are in the required set, but doesn't verify if all required headers are present or validate their values.
def test_scanoss_generic_headers(self): - scanoss_api = ScanossApi(debug=True, req_headers={'x-api-key': '123455', 'generic-header': 'generic-header-value'}) + api_key = '123455' + header_value = 'generic-header-value' + scanoss_api = ScanossApi( + debug=True, + req_headers={'x-api-key': api_key, 'generic-header': header_value} + ) required_keys = ('x-api-key', 'User-Agent', 'user-agent', 'generic-header') - valid_headers = True + + # Check all header keys are in required keys for key, value in scanoss_api.headers.items(): - if key not in required_keys: - valid_headers = False - self.assertTrue(valid_headers) + self.assertIn(key, required_keys, f"Unexpected header key: {key}") + + # Check all required keys are in headers + for key in required_keys: + if key != 'User-Agent': # Skip User-Agent check since it might be lowercase + self.assertIn(key, scanoss_api.headers, f"Required header key missing: {key}") + + # Verify specific values + self.assertEqual(scanoss_api.headers.get('x-api-key'), api_key) + self.assertEqual(scanoss_api.headers.get('generic-header'), header_value)🧰 Tools
🪛 Ruff (0.8.2)
31-31: Line too long (123 > 120)
(E501)
🪛 GitHub Actions: Lint
[error] 31-31: Line too long (123 > 120).
src/scanoss/cli.py (1)
1476-1499: Enhance header parsing with better error handlingThe
process_req_headersfunction is well-structured but could use some improvements:
- It silently ignores malformed header strings (missing colon)
- Uses a magic value
2for comparisonConsider this improved implementation:
def process_req_headers(headers_array: List[str]) -> dict: """ Process a list of header strings in the format "Name: Value" into a dictionary. Args: headers_array (list): List of header strings from command line args Returns: dict: Dictionary of header name-value pairs """ # Check if headers_array is empty if not headers_array: # Array is empty return {} dict_headers = {} + HEADER_PARTS_COUNT = 2 for header_str in headers_array: # Split each "Name: Value" header parts = header_str.split(":", 1) - if len(parts) == 2: + if len(parts) == HEADER_PARTS_COUNT: name = parts[0].strip() value = parts[1].strip() dict_headers[name] = value + else: + print(f"Warning: Ignoring malformed header: {header_str}", file=sys.stderr) return dict_headers🧰 Tools
🪛 Ruff (0.8.2)
1495-1495: Magic value used in comparison, consider replacing
2with a constant variable(PLR2004)
🪛 GitHub Actions: Lint
[error] 1495-1495: Magic value used in comparison, consider replacing
2with a constant variable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
CHANGELOG.md(2 hunks)CLIENT_HELP.md(2 hunks)src/scanoss/__init__.py(1 hunks)src/scanoss/cli.py(12 hunks)src/scanoss/components.py(2 hunks)src/scanoss/scanner.py(4 hunks)src/scanoss/scanossapi.py(3 hunks)src/scanoss/scanossgrpc.py(3 hunks)tests/cli-test.py(1 hunks)tests/grpc-client-test.py(1 hunks)tests/scanossapi-test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/scanoss/init.py
- CLIENT_HELP.md
- src/scanoss/components.py
- src/scanoss/scanner.py
- src/scanoss/scanossgrpc.py
🧰 Additional context used
🧬 Code Definitions (3)
src/scanoss/scanossapi.py (1)
src/scanoss/scanossgrpc.py (1) (1)
load_generic_headers(518-531)
tests/grpc-client-test.py (1)
src/scanoss/scanossgrpc.py (1) (1)
ScanossGrpc(71-531)
tests/scanossapi-test.py (3)
src/scanoss/scanossapi.py (1) (1)
ScanossApi(49-285)tests/cli-test.py (1) (1)
MyTestCase(28-60)tests/grpc-client-test.py (1) (1)
MyTestCase(36-131)
🪛 Ruff (0.8.2)
src/scanoss/scanossapi.py
95-95: Magic value used in comparison, consider replacing 5 with a constant variable
(PLR2004)
src/scanoss/cli.py
28-28: array.array imported but unused
Remove unused import: array.array
(F401)
29-29: cgi.parse_header imported but unused
Remove unused import: cgi.parse_header
(F401)
1495-1495: Magic value used in comparison, consider replacing 2 with a constant variable
(PLR2004)
tests/cli-test.py
52-52: Line too long (121 > 120)
(E501)
tests/grpc-client-test.py
125-125: Line too long (124 > 120)
(E501)
tests/scanossapi-test.py
31-31: Line too long (123 > 120)
(E501)
🪛 GitHub Actions: Lint
src/scanoss/scanossapi.py
[warning] 25-25: Import block is un-sorted or un-formatted. Organize imports.
[error] 55-55: Too many arguments in function definition (15 > 5).
[error] 95-95: Magic value used in comparison, consider replacing 5 with a constant variable.
[error] 119-119: f-string without any placeholders.
[error] 125-125: f-string without any placeholders.
[error] 136-136: Too many branches (23 > 12).
[error] 136-136: Too many statements (76 > 50).
[error] 195-195: Magic value used in comparison, consider replacing 503 with a constant variable.
[error] 205-205: Magic value used in comparison, consider replacing 400 with a constant variable.
src/scanoss/cli.py
[warning] 25-25: Import block is un-sorted or un-formatted. Organize imports.
[error] 28-28: array.array imported but unused. Remove unused import: array.array.
[error] 29-29: cgi.parse_header imported but unused. Remove unused import: cgi.parse_header.
[error] 1210-1210: String strip call contains duplicate characters.
[error] 1495-1495: Magic value used in comparison, consider replacing 2 with a constant variable.
tests/cli-test.py
[warning] 25-25: Import block is un-sorted or un-formatted. Organize imports.
[error] 52-52: Line too long (121 > 120).
tests/grpc-client-test.py
[warning] 25-25: Import block is un-sorted or un-formatted. Organize imports.
[error] 125-125: Line too long (124 > 120).
tests/scanossapi-test.py
[warning] 25-25: Import block is un-sorted or un-formatted. Organize imports.
[error] 31-31: Line too long (123 > 120).
🪛 LanguageTool
CHANGELOG.md
[duplication] ~13-~13: Possible typo: you repeated a word.
Context: ...hanges... ## [1.20.6] - 2025-03-19 ### Added - Added HTTP/gRPC generic headers ## [1.20.5] ...
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (9)
CHANGELOG.md (2)
12-15: LGTM! Version and changelog entry look goodThe addition of the new version with HTTP/gRPC generic headers is well documented and follows the project's changelog format.
🧰 Tools
🪛 LanguageTool
[duplication] ~13-~13: Possible typo: you repeated a word.
Context: ...hanges... ## [1.20.6] - 2025-03-19 ### Added - Added HTTP/gRPC generic headers ## [1.20.5] ...(ENGLISH_WORD_REPEAT_RULE)
492-493: LGTM! Version comparison links are correctly updatedThe links for comparing versions have been properly updated to include the new version.
tests/cli-test.py (1)
30-60: LGTM! Comprehensive test for header processingThe test covers both valid and malformed headers, and thoroughly validates the expected behavior of the
process_req_headersfunction.🧰 Tools
🪛 Ruff (0.8.2)
52-52: Line too long (121 > 120)
(E501)
🪛 GitHub Actions: Lint
[error] 52-52: Line too long (121 > 120).
src/scanoss/scanossapi.py (3)
71-71: Good addition of thereq_headersparameterAdding the
req_headersparameter with a default value ofNoneensures backward compatibility while enabling custom HTTP header support.
98-98: Good defensive programming with req_headers initializationProperly initializing
self.req_headerswith a fallback to an empty dictionary if the parameter is None is a good defensive programming practice.
272-285: Well-structured method for handling custom headersThe
load_generic_headersmethod is well-designed with clear documentation and proper handling of the special case for thex-api-keyheader.However, be aware that this implementation may not behave as expected due to the initialization ordering issue noted in an earlier comment.
src/scanoss/cli.py (3)
573-578: Well-designed header command-line argumentThe
--header/-hdrargument is well-designed:
- Can be specified multiple times (
action='append')- Clear help text explaining the expected format
- Consistent with other argument definitions
This provides users with a convenient way to add custom headers to requests.
719-720: Consistent integration of req_headers in wfp functionThe integration of processed request headers is done consistently here and across the codebase.
916-917: Consistent integration of req_headers in scan functionThe integration of processed request headers is done consistently.
tests/grpc-client-test.py
Outdated
| @@ -121,5 +121,16 @@ def test_load_purls_file(self): | |||
| # Ensure the method returned None (indicating a failure) | |||
| self.assertEqual(components,expected_value) | |||
|
|
|||
| def test_grpc_generic_metadata(self): | |||
| grpc_client = ScanossGrpc(debug=True, req_headers={'x-api-key': '123455', 'generic-header': 'generic-header-value'}) | |||
There was a problem hiding this comment.
Fix line length to comply with style guidelines
This line exceeds the 120 character limit specified in your style guide.
- grpc_client = ScanossGrpc(debug=True, req_headers={'x-api-key': '123455', 'generic-header': 'generic-header-value'})
+ grpc_client = ScanossGrpc(
+ debug=True,
+ req_headers={'x-api-key': '123455', 'generic-header': 'generic-header-value'}
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| grpc_client = ScanossGrpc(debug=True, req_headers={'x-api-key': '123455', 'generic-header': 'generic-header-value'}) | |
| grpc_client = ScanossGrpc( | |
| debug=True, | |
| req_headers={'x-api-key': '123455', 'generic-header': 'generic-header-value'} | |
| ) |
🧰 Tools
🪛 Ruff (0.8.2)
125-125: Line too long (124 > 120)
(E501)
🪛 GitHub Actions: Lint
[error] 125-125: Line too long (124 > 120).
| import unittest | ||
| from scanoss.scanossapi import ScanossApi |
There was a problem hiding this comment.
Fix import sorting to comply with linting rules
The import block is not properly sorted according to the linter.
-import unittest
-from scanoss.scanossapi import ScanossApi
+import unittest
+
+from scanoss.scanossapi import ScanossApi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import unittest | |
| from scanoss.scanossapi import ScanossApi | |
| import unittest | |
| from scanoss.scanossapi import ScanossApi |
🧰 Tools
🪛 GitHub Actions: Lint
[warning] 25-25: Import block is un-sorted or un-formatted. Organize imports.
tests/scanossapi-test.py
Outdated
| class MyTestCase(unittest.TestCase): | ||
|
|
||
| def test_scanoss_generic_headers(self): | ||
| scanoss_api = ScanossApi(debug=True, req_headers={'x-api-key': '123455', 'generic-header': 'generic-header-value'}) |
There was a problem hiding this comment.
Fix line length to comply with style guidelines
This line exceeds the 120 character limit specified in your style guide.
- scanoss_api = ScanossApi(debug=True, req_headers={'x-api-key': '123455', 'generic-header': 'generic-header-value'})
+ scanoss_api = ScanossApi(
+ debug=True,
+ req_headers={'x-api-key': '123455', 'generic-header': 'generic-header-value'}
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| scanoss_api = ScanossApi(debug=True, req_headers={'x-api-key': '123455', 'generic-header': 'generic-header-value'}) | |
| scanoss_api = ScanossApi( | |
| debug=True, | |
| req_headers={'x-api-key': '123455', 'generic-header': 'generic-header-value'} | |
| ) |
🧰 Tools
🪛 Ruff (0.8.2)
31-31: Line too long (123 > 120)
(E501)
🪛 GitHub Actions: Lint
[error] 31-31: Line too long (123 > 120).
tests/cli-test.py
Outdated
| self.assertEqual(processed_headers, expected_headers, | ||
| f"Headers don't match expected values.\nGot: {processed_headers}\nExpected: {expected_headers}") |
There was a problem hiding this comment.
Fix line length to comply with style guidelines
This line exceeds the 120 character limit specified in your style guide.
- self.assertEqual(processed_headers, expected_headers,
- f"Headers don't match expected values.\nGot: {processed_headers}\nExpected: {expected_headers}")
+ error_msg = f"Headers don't match expected values.\nGot: {processed_headers}\nExpected: {expected_headers}"
+ self.assertEqual(processed_headers, expected_headers, error_msg)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.assertEqual(processed_headers, expected_headers, | |
| f"Headers don't match expected values.\nGot: {processed_headers}\nExpected: {expected_headers}") | |
| error_msg = f"Headers don't match expected values.\nGot: {processed_headers}\nExpected: {expected_headers}" | |
| self.assertEqual(processed_headers, expected_headers, error_msg) |
🧰 Tools
🪛 Ruff (0.8.2)
52-52: Line too long (121 > 120)
(E501)
🪛 GitHub Actions: Lint
[error] 52-52: Line too long (121 > 120).
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/scanoss/cli.py (1)
1208-1208: 💡 Verification agent🧩 Analysis chain
Fix duplicate characters in string strip function
The pipeline has flagged a duplicate character in the strip function elsewhere in the file.
🏁 Script executed:
#!/bin/bash # Look for the strip function with duplicate characters on line 1208 sed -n '1208p' src/scanoss/cli.pyLength of output: 78
Action Required: Remove Duplicate Character in Strip Argument
Please update the string literal used with the strip function so that it includes only unique characters. For example, change:
pac_local = pac.strip('file://')to
pac_local = pac.strip('file:/')This adjustment removes the redundant '/' and resolves the pipeline flag.
- File:
src/scanoss/cli.py, Line: 1208🧰 Tools
🪛 GitHub Actions: Lint
[error] 1208-1208: String
stripcall contains duplicate characters.
♻️ Duplicate comments (1)
src/scanoss/cli.py (1)
28-29:⚠️ Potential issueRemove unused imports
The imports
array.arrayandcgi.parse_headerare not used in the code and should be removed.-from array import array -from cgi import parse_header🧰 Tools
🪛 Ruff (0.8.2)
28-28:
array.arrayimported but unusedRemove unused import:
array.array(F401)
29-29:
cgi.parse_headerimported but unusedRemove unused import:
cgi.parse_header(F401)
🪛 GitHub Actions: Lint
[error] 28-28:
array.arrayimported but unused. Remove unused import:array.array.
[error] 29-29:
cgi.parse_headerimported but unused. Remove unused import:cgi.parse_header.
🧹 Nitpick comments (2)
src/scanoss/cli.py (2)
1474-1497: Refine header processing implementationThe function correctly handles the basic header processing, but has a few areas for improvement:
- It uses a magic number in the comparison
- It silently ignores malformed headers
- It doesn't handle the case when the same header name appears multiple times
Consider this improved implementation:
def process_req_headers(headers_array: List[str]) -> dict: """ Process a list of header strings in the format "Name: Value" into a dictionary. Args: headers_array (list): List of header strings from command line args Returns: dict: Dictionary of header name-value pairs """ # Check if headers_array is empty if not headers_array: # Array is empty return {} + HEADER_PARTS_COUNT = 2 dict_headers = {} for header_str in headers_array: # Split each "Name: Value" header parts = header_str.split(":", 1) - if len(parts) == 2: + if len(parts) == HEADER_PARTS_COUNT: name = parts[0].strip() value = parts[1].strip() dict_headers[name] = value + else: + print_stderr(f"Warning: Ignoring malformed header: '{header_str}'. Expected format: 'Name: Value'") return dict_headers🧰 Tools
🪛 Ruff (0.8.2)
1493-1493: Magic value used in comparison, consider replacing
2with a constant variable(PLR2004)
🪛 GitHub Actions: Lint
[error] 1493-1493: Magic value used in comparison, consider replacing
2with a constant variable.
1485-1487: Consider simplifying the empty checkThe function already handles the None/empty case well, but the comment is redundant.
# Check if headers_array is empty if not headers_array: - # Array is empty return {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/scanoss/cli.py(11 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/scanoss/cli.py
28-28: array.array imported but unused
Remove unused import: array.array
(F401)
29-29: cgi.parse_header imported but unused
Remove unused import: cgi.parse_header
(F401)
1493-1493: Magic value used in comparison, consider replacing 2 with a constant variable
(PLR2004)
🪛 GitHub Actions: Lint
src/scanoss/cli.py
[warning] 25-25: Import block is un-sorted or un-formatted. Organize imports.
[error] 28-28: array.array imported but unused. Remove unused import: array.array.
[error] 29-29: cgi.parse_header imported but unused. Remove unused import: cgi.parse_header.
[error] 1208-1208: String strip call contains duplicate characters.
[error] 1493-1493: Magic value used in comparison, consider replacing 2 with a constant variable.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
src/scanoss/cli.py (3)
573-578: Good addition of header supportThe command-line argument for custom headers is well-implemented, allowing users to specify multiple headers using the
-hdror--headeroption with the correctaction='append'parameter.
914-914: Scanner initialization updated correctlyThe
req_headersparameter is properly passed to the Scanner initialization, integrating the new header functionality.
1252-1252: Header parameters consistently applied across componentsHeaders are consistently passed to all relevant component functions, ensuring the feature works across the entire system.
Also applies to: 1287-1287, 1322-1322, 1360-1360, 1405-1405, 1430-1430
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/scanoss/scanossapi.py (1)
90-98: 🛠️ Refactor suggestionParameter initialization needs refactoring
There's still redundancy in setting
self.urlandself.api_key- they're initialized here and then again at lines 110-111, which could cause confusion.- self.url = url - self.api_key = api_key self.sbom = None self.scan_format = scan_format if scan_format else 'plain' self.flags = flags self.timeout = timeout if timeout > 5 else 180 self.retry_limit = retry if retry >= 0 else 5 self.ignore_cert_errors = ignore_cert_errors self.req_headers = req_headers if req_headers else {}🧰 Tools
🪛 Ruff (0.8.2)
95-95: Magic value used in comparison, consider replacing
5with a constant variable(PLR2004)
🪛 GitHub Actions: Lint
[error] 95-95: Magic value used in comparison, consider replacing
5with a constant variable.
🧹 Nitpick comments (3)
src/scanoss/scanossapi.py (3)
125-125: Remove unnecessary f-string prefixThe string doesn't contain any placeholders, so the
fprefix is unnecessary.- self.print_debug(f'Ignoring cert errors...') + self.print_debug('Ignoring cert errors...')🧰 Tools
🪛 Ruff (0.8.2)
125-125: f-string without any placeholders
Remove extraneous
fprefix(F541)
🪛 GitHub Actions: Lint
[error] 125-125: f-string without any placeholders. Remove extraneous
fprefix.
195-205: Replace magic numbers with named constantsReplace the magic values
503and400with named constants to improve code readability and maintainability, as flagged by the pipeline.+ # HTTP status codes + SERVICE_UNAVAILABLE = 503 + BAD_REQUEST = 400 - elif r.status_code == 503: # Service limits have most likely been reached + elif r.status_code == SERVICE_UNAVAILABLE: # Service limits have most likely been reached self.print_stderr( f'ERROR: SCANOSS API rejected the scan request ({request_id}) due to ' f'service limits being exceeded' ) self.print_stderr(f'ERROR: Details: {r.text.strip()}') raise Exception( f'ERROR: {r.status_code} - The SCANOSS API request ({request_id}) rejected ' f'for {self.url} due to service limits being exceeded.' ) - elif r.status_code >= 400: + elif r.status_code >= BAD_REQUEST:🧰 Tools
🪛 Ruff (0.8.2)
195-195: Magic value used in comparison, consider replacing
503with a constant variable(PLR2004)
205-205: Magic value used in comparison, consider replacing
400with a constant variable(PLR2004)
🪛 GitHub Actions: Lint
[error] 195-195: Magic value used in comparison, consider replacing
503with a constant variable.
[error] 205-205: Magic value used in comparison, consider replacing
400with a constant variable.
136-136: Consider refactoring the scan methodThe
scanmethod exceeds the recommended complexity limits with too many branches (23 > 12) and too many statements (76 > 50). Consider breaking it down into smaller, more focused methods.You could split the method into separate methods:
- A method to prepare the request data
- A method to handle the retry logic
- A method to process the response
This would improve readability and maintainability.
🧰 Tools
🪛 Ruff (0.8.2)
136-136: Too many branches (23 > 12)
(PLR0912)
136-136: Too many statements (76 > 50)
(PLR0915)
🪛 GitHub Actions: Lint
[error] 136-136: Too many branches (23 > 12).
[error] 136-136: Too many statements (76 > 50).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/scanoss/scanner.py(18 hunks)src/scanoss/scanossapi.py(4 hunks)src/scanoss/scanossgrpc.py(4 hunks)tests/cli-test.py(1 hunks)tests/grpc-client-test.py(1 hunks)tests/scanossapi-test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/grpc-client-test.py
- tests/scanossapi-test.py
- src/scanoss/scanner.py
- src/scanoss/scanossgrpc.py
🧰 Additional context used
🧬 Code Definitions (2)
tests/cli-test.py (2)
tests/grpc-client-test.py (1) (1)
MyTestCase(36-132)tests/scanossapi-test.py (1) (1)
MyTestCase(28-38)
src/scanoss/scanossapi.py (1)
src/scanoss/scanossgrpc.py (1) (1)
load_generic_headers(518-531)
🪛 Ruff (0.8.2)
src/scanoss/scanossapi.py
95-95: Magic value used in comparison, consider replacing 5 with a constant variable
(PLR2004)
🪛 GitHub Actions: Lint
src/scanoss/scanossapi.py
[warning] 25-25: Import block is un-sorted or un-formatted. Organize imports.
[error] 95-95: Magic value used in comparison, consider replacing 5 with a constant variable.
[error] 125-125: f-string without any placeholders. Remove extraneous f prefix.
[error] 136-136: Too many branches (23 > 12).
[error] 136-136: Too many statements (76 > 50).
[error] 195-195: Magic value used in comparison, consider replacing 503 with a constant variable.
[error] 205-205: Magic value used in comparison, consider replacing 400 with a constant variable.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (6)
tests/cli-test.py (3)
25-27: Import structure looks goodThe imports are properly formatted with a standard import first, followed by a blank line, then the specific module import.
30-66: Well-structured test case for header processingThis test case thoroughly validates the
process_req_headersfunction, covering important scenarios:
- Valid headers with proper formatting
- Headers with surrounding whitespace
- Malformed headers without colon separators
The assertions are comprehensive, checking exact dictionary equality, presence of required headers, and exclusion of malformed headers.
52-55: Line length issue properly addressedThe error message has been nicely split across multiple lines to address the previous line length issue.
src/scanoss/scanossapi.py (3)
55-71: New parameter correctly added to method signatureThe
req_headersparameter has been properly added to the constructor with appropriate type annotation and default value.
108-114: Header loading and URL initialization sequence improvedThe sequence has been improved to ensure headers are loaded before finalizing URL and API key values, addressing the previous review comments.
272-285: Well-implemented custom header loading methodThe
load_generic_headersmethod properly handles custom headers, including special processing for thex-api-keyheader. The method follows the same pattern as implemented inscanossgrpc.py, maintaining consistency across the codebase.
| if self.api_key and not url and not os.environ.get('SCANOSS_SCAN_URL'): | ||
| self.url = DEFAULT_URL2 # API key specific and no alternative URL, so use the default premium | ||
| self.url = url | ||
| self.api_key = api_key | ||
| self.sbom = None | ||
| self.scan_format = scan_format if scan_format else 'plain' | ||
| self.flags = flags | ||
| self.timeout = timeout if timeout > 5 else 180 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Replace magic number with a named constant
Replace the magic value 5 with a named constant to improve code readability and maintainability.
+ MIN_TIMEOUT = 5
- self.timeout = timeout if timeout > 5 else 180
+ self.timeout = timeout if timeout > MIN_TIMEOUT else 180🏁 Script executed:
#!/bin/bash
# Check if the magic number 5 is used consistently across the codebase
rg "timeout.+if.+>.+5" --type pyLength of output: 115
Replace magic number with a named constant
The verification confirms that the magic number 5 appears only in src/scanoss/scanossapi.py at line 95. Please replace the literal 5 with a named constant for better readability and maintainability. For example:
+ MIN_TIMEOUT = 5
- self.timeout = timeout if timeout > 5 else 180
+ self.timeout = timeout if timeout > MIN_TIMEOUT else 180📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.timeout = timeout if timeout > 5 else 180 | |
| MIN_TIMEOUT = 5 | |
| self.timeout = timeout if timeout > MIN_TIMEOUT else 180 |
🧰 Tools
🪛 Ruff (0.8.2)
95-95: Magic value used in comparison, consider replacing 5 with a constant variable
(PLR2004)
🪛 GitHub Actions: Lint
[error] 95-95: Magic value used in comparison, consider replacing 5 with a constant variable.
agustingroh
force-pushed
the
feat/groh/add-generic-http-grpc-header
branch
from
6450cb0 to
5712262
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/scanoss/scanossapi.py (1)
272-285:⚠️ Potential issueIncorrect environment variable name in URL check
Line 282 uses
SCANOSS_GRPC_URLbut should be usingSCANOSS_SCAN_URL, since this is the REST API client class, not the gRPC client.- if not self.url and not os.environ.get('SCANOSS_GRPC_URL'): + if not self.url and not os.environ.get('SCANOSS_SCAN_URL'):
🧹 Nitpick comments (1)
src/scanoss/cli.py (1)
31-31: Add typing imports directly to import blockConsider adding
Listto the existing imports instead of including a separate import line for the typing module. It's more organized and follows standard Python import conventions.-from typing import List +import sys +from pathlib import Path +from typing import List +import pypac
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/scanoss/cli.py(12 hunks)src/scanoss/components.py(5 hunks)src/scanoss/scanner.py(20 hunks)src/scanoss/scanossapi.py(4 hunks)src/scanoss/scanossgrpc.py(4 hunks)tests/cli-test.py(1 hunks)tests/grpc-client-test.py(1 hunks)tests/scanossapi-test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/grpc-client-test.py
- tests/cli-test.py
- src/scanoss/components.py
- tests/scanossapi-test.py
- src/scanoss/scanossgrpc.py
- src/scanoss/scanner.py
🧰 Additional context used
🪛 GitHub Actions: Lint
src/scanoss/cli.py
[warning] 25-25: I001 [*] Import block is un-sorted or un-formatted. Help: Organize imports.
[warning] 1207-1207: PLE1310 String strip call contains duplicate characters.
src/scanoss/scanossapi.py
[warning] 25-25: I001 [*] Import block is un-sorted or un-formatted. Help: Organize imports.
[warning] 95-95: PLR2004 Magic value used in comparison, consider replacing 5 with a constant variable.
[warning] 136-136: PLR0912 Too many branches (23 > 12).
[warning] 136-136: PLR0915 Too many statements (76 > 50).
[warning] 195-195: PLR2004 Magic value used in comparison, consider replacing 503 with a constant variable.
[warning] 205-205: PLR2004 Magic value used in comparison, consider replacing 400 with a constant variable.
🪛 Ruff (0.8.2)
src/scanoss/scanossapi.py
95-95: Magic value used in comparison, consider replacing 5 with a constant variable
(PLR2004)
🔇 Additional comments (10)
src/scanoss/cli.py (5)
54-54: Good use of a named constantAdding
HEADER_PARTS_COUNTas a named constant rather than using a magic number in the code improves maintainability and readability.
572-577: Well-implemented CLI argument for HTTP headersThe header argument implementation is well-designed:
- It uses
action='append'to support multiple headers- It provides both long (
--header) and short (-hdr) forms- The help text clearly explains the expected format and usage
913-913: Properly passes headers to the scannerThe implementation correctly processes request headers and passes them to the scanner.
1239-1252: Consistent parameter passing to the Components classThe implementation correctly passes the processed headers to the Components class.
1473-1496: Robust header processing implementationThe
process_req_headersfunction is well implemented with:
- Proper type hints for parameters and return value
- Good documentation explaining the purpose and format of input/output
- Handles the empty case appropriately
- Uses the named constant for clarity
- Properly strips whitespace from both header names and values
src/scanoss/scanossapi.py (5)
71-71: Good addition of request headers parameterAdding the
req_headersparameter with a proper type hint and default value is a good approach for extending the functionality without breaking existing code.
95-95: Replace magic number with a named constantReplace the hardcoded value of
5with a named constant for better readability and maintainability.+ MIN_TIMEOUT_VALUE = 5 - self.timeout = timeout if timeout > 5 else 180 + self.timeout = timeout if timeout > MIN_TIMEOUT_VALUE else 180🧰 Tools
🪛 Ruff (0.8.2)
95-95: Magic value used in comparison, consider replacing
5with a constant variable(PLR2004)
🪛 GitHub Actions: Lint
[warning] 95-95: PLR2004 Magic value used in comparison, consider replacing
5with a constant variable.
98-98: Proper initialization of req_headers attributeGood practice setting the default to an empty dictionary if no headers are provided.
108-108: Appropriate placement of header loading methodLoading headers here is appropriate as it needs to happen before the URL and API key initialization that follows.
110-114: URL and API key initialization improvedThe initialization logic has been improved to address the redundancy that was previously flagged in the past review comments.
|
@eeisegn Just updated the PR based on your comments, let me know if I can merge it |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/scanoss/scanossapi.py (1)
272-285: Clear header processing implementationThe
load_generic_headersmethod is well-designed with clear documentation. It correctly processes custom headers and handles the special case forx-api-key.However, the method should be more explicit about modifying class properties:
def load_generic_headers(self): """ Adds custom headers from req_headers to the headers collection. If x-api-key is present and no URL is configured (directly or via environment), sets URL to the premium endpoint (DEFAULT_URL2). + + Note: This method may modify self.url and self.api_key based on header contents. """ if self.req_headers: # Load generic headers for key, value in self.req_headers.items():CHANGELOG.md (1)
14-14: Nitpick: Redundant “Added” WordingThe bullet point on line 14 starts with "Added" even though the section header already implies an addition. Consider rephrasing the line (for example, removing the leading verb) to avoid redundancy and improve conciseness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(2 hunks)CLIENT_HELP.md(2 hunks)src/scanoss/scanossapi.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CLIENT_HELP.md
🧰 Additional context used
🪛 Ruff (0.8.2)
src/scanoss/scanossapi.py
95-95: Magic value used in comparison, consider replacing 5 with a constant variable
(PLR2004)
🪛 GitHub Actions: Lint
src/scanoss/scanossapi.py
[warning] 25-25: I001 [*] Import block is un-sorted or un-formatted. Help: Organize imports.
[warning] 95-95: PLR2004 Magic value used in comparison, consider replacing 5 with a constant variable.
[warning] 136-136: PLR0912 Too many branches (23 > 12).
[warning] 136-136: PLR0915 Too many statements (76 > 50).
[warning] 195-195: PLR2004 Magic value used in comparison, consider replacing 503 with a constant variable.
[warning] 205-205: PLR2004 Magic value used in comparison, consider replacing 400 with a constant variable.
🪛 LanguageTool
CHANGELOG.md
[duplication] ~13-~13: Possible typo: you repeated a word.
Context: ...hanges... ## [1.20.6] - 2025-03-19 ### Added - Added HTTP/gRPC generic headers feature using...
(ENGLISH_WORD_REPEAT_RULE)
🔇 Additional comments (6)
src/scanoss/scanossapi.py (5)
55-71: Well structured parameter additionThe addition of the
req_headersparameter to the__init__method is a clean implementation that preserves backward compatibility by providing a default value ofNone. This aligns well with the PR objective of supporting custom HTTP headers.
90-98: Good initialization of request headersThe initialization of request headers is implemented well. Setting
self.req_headersto the provided value or defaulting to an empty dictionary is a good approach.🧰 Tools
🪛 Ruff (0.8.2)
95-95: Magic value used in comparison, consider replacing
5with a constant variable(PLR2004)
🪛 GitHub Actions: Lint
[warning] 95-95: PLR2004 Magic value used in comparison, consider replacing
5with a constant variable.
95-95: Replace magic number with a named constantThe magic number
5in the condition is flagged by static analysis and should be replaced with a named constant for better readability and maintainability.+ MIN_TIMEOUT = 5 - self.timeout = timeout if timeout > 5 else 180 + self.timeout = timeout if timeout > MIN_TIMEOUT else 180🧰 Tools
🪛 Ruff (0.8.2)
95-95: Magic value used in comparison, consider replacing
5with a constant variable(PLR2004)
🪛 GitHub Actions: Lint
[warning] 95-95: PLR2004 Magic value used in comparison, consider replacing
5with a constant variable.
108-114: Improved initialization flow with headers loadingThe reordering of initialization ensures headers are loaded first, then URL and API key are set with proper precedence. This addresses the past review comments about redundancy and potential conflicts in setting these properties.
282-282: Consistent environment variable checkThe condition checks for the correct environment variable
SCANOSS_SCAN_URL, which is consistent with how this variable is used elsewhere in the code.CHANGELOG.md (1)
492-493: Version Comparison Links UpdateThe updated links for versions [1.20.5] and [1.20.6] are correctly formatted. They provide clear comparison URLs between releases, enhancing the changelog’s usability.
| ## [1.20.6] - 2025-03-19 | ||
| ### Added | ||
| - Added HTTP/gRPC generic headers feature using --header flag | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
New Version Entry for HTTP/gRPC Headers Feature
The new section for version [1.20.6] clearly documents the addition of the generic HTTP/gRPC headers feature using the --header flag. Ensure that the description here aligns with the feature’s implementation in the CLI, API, and GRPC modules.
🧰 Tools
🪛 LanguageTool
[duplication] ~13-~13: Possible typo: you repeated a word.
Context: ...hanges... ## [1.20.6] - 2025-03-19 ### Added - Added HTTP/gRPC generic headers feature using...
(ENGLISH_WORD_REPEAT_RULE)
Summary by CodeRabbit
Documentation
New Features
Tests
Changelog
Version Update