-
Notifications
You must be signed in to change notification settings - Fork 24
Fix linting errors part 2 #211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5546f01
9b31c42
9d092cb
5e14aeb
3a33cd2
b858089
d26551a
104f405
74a34d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |||||||||||||||||||||||||||||||
| # SPDX-License-Identifier: Apache-2.0 | ||||||||||||||||||||||||||||||||
| # ******************************************************************************* | ||||||||||||||||||||||||||||||||
| import re | ||||||||||||||||||||||||||||||||
| from os import error | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| from score_metamodel import ( | ||||||||||||||||||||||||||||||||
| CheckLogger, | ||||||||||||||||||||||||||||||||
|
|
@@ -33,6 +34,30 @@ def get_need_type(needs_types: list[ScoreNeedType], directive: str) -> ScoreNeed | |||||||||||||||||||||||||||||||
| raise ValueError(f"Need type {directive} not found in needs_types") | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| def _normalize_values(raw_value: str | list[str] | None) -> list[str]: | ||||||||||||||||||||||||||||||||
| """Normalize a raw value into a list of strings.""" | ||||||||||||||||||||||||||||||||
|
MaximilianSoerenPollak marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||
| if raw_value is None: | ||||||||||||||||||||||||||||||||
| return [] | ||||||||||||||||||||||||||||||||
| if isinstance(raw_value, str): | ||||||||||||||||||||||||||||||||
| return [raw_value] | ||||||||||||||||||||||||||||||||
| if isinstance(raw_value, list) and all(isinstance(v, str) for v in raw_value): | ||||||||||||||||||||||||||||||||
| return raw_value | ||||||||||||||||||||||||||||||||
| raise ValueError | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| def _validate_value_pattern( | ||||||||||||||||||||||||||||||||
| value: str, pattern: str, need: NeedsInfoType, field: str, log: CheckLogger | ||||||||||||||||||||||||||||||||
| ) -> None: | ||||||||||||||||||||||||||||||||
| """Check if a value matches the given pattern, log warnings if not.""" | ||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||
| if not re.match(pattern, value): | ||||||||||||||||||||||||||||||||
| log.warning_for_option(need, field, f"does not follow pattern `{pattern}`.") | ||||||||||||||||||||||||||||||||
| except TypeError: | ||||||||||||||||||||||||||||||||
| log.warning_for_option( | ||||||||||||||||||||||||||||||||
| need, field, f"pattern `{pattern}` is not a valid regex pattern." | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| def validate_fields( | ||||||||||||||||||||||||||||||||
| need: NeedsInfoType, | ||||||||||||||||||||||||||||||||
| log: CheckLogger, | ||||||||||||||||||||||||||||||||
|
|
@@ -64,31 +89,18 @@ def remove_prefix(word: str, prefixes: list[str]) -> str: | |||||||||||||||||||||||||||||||
| need, f"is missing required {field_type}: `{field}`." | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
| continue # Skip empty optional fields | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| values: list[str] | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| if isinstance(raw_value, str): | ||||||||||||||||||||||||||||||||
| values = [raw_value] | ||||||||||||||||||||||||||||||||
| elif isinstance(raw_value, list) and all(isinstance(v, str) for v in raw_value): | ||||||||||||||||||||||||||||||||
| values = raw_value | ||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||
| values = [str(raw_value)] | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||
| values = _normalize_values(raw_value) | ||||||||||||||||||||||||||||||||
| except ValueError as err: | ||||||||||||||||||||||||||||||||
| raise ValueError( | ||||||||||||||||||||||||||||||||
| f"An Attribute inside need {need['id']} is " | ||||||||||||||||||||||||||||||||
| "not of type str. Only Strings are allowed" | ||||||||||||||||||||||||||||||||
| ) from err | ||||||||||||||||||||||||||||||||
|
Comment on lines
+92
to
+98
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit (fix in next PR): Add comment explaining why we did this.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||||||||||||||||||||||||||||||||
| # The filter ensures that the function is only called when needed. | ||||||||||||||||||||||||||||||||
| for value in values: | ||||||||||||||||||||||||||||||||
| if allowed_prefixes: | ||||||||||||||||||||||||||||||||
| value = remove_prefix(value, allowed_prefixes) | ||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||
| if not re.match(pattern, value): | ||||||||||||||||||||||||||||||||
| log.warning_for_option( | ||||||||||||||||||||||||||||||||
| need, field, f"does not follow pattern `{pattern}`." | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
| except TypeError: | ||||||||||||||||||||||||||||||||
| log.warning_for_option( | ||||||||||||||||||||||||||||||||
| need, | ||||||||||||||||||||||||||||||||
| field, | ||||||||||||||||||||||||||||||||
| f"pattern `{pattern}` is not a valid regex pattern.", | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
| _validate_value_pattern(value, pattern, need, field, log) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| # req-Id: tool_req__docs_req_attr_reqtype | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -156,7 +156,7 @@ def test_invalid_option_type(self): | |
| target_id="wf_req__001", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit (fix in next PR): Add a test that now tests the new 'value error' path we have introduced
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
| id="wf_req__001", | ||
| type="workflow", | ||
| some_invalid_option=42, | ||
| some_invalid_option="42", | ||
| docname=None, | ||
| lineno=None, | ||
| ) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # ******************************************************************************* | ||
| import contextlib | ||
| import json | ||
| import os | ||
| import shutil | ||
|
|
@@ -150,14 +151,11 @@ def _create_app(): | |
| base_dir = sphinx_base_dir | ||
| docs_dir = base_dir / "docs" | ||
|
|
||
| original_cwd = None | ||
| # CRITICAL: Change to a directory that exists and is accessible | ||
| # This fixes the "no such file or directory" error in Bazel | ||
| original_cwd = None | ||
| try: | ||
| with contextlib.suppress(FileNotFoundError): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit (fix in next PR): Comment what this does briefly as people might not be familliar with this.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I kind of disagree. We should not explain standard python functionality. Python programmers must know python.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That does make more sense, true. Let's do that.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
| original_cwd = os.getcwd() | ||
| except FileNotFoundError: | ||
| # Current working directory doesn't exist, which is the problem | ||
| pass | ||
|
|
||
| # Change to the base_dir before creating SphinxTestApp | ||
| os.chdir(base_dir) | ||
|
|
@@ -173,11 +171,9 @@ def _create_app(): | |
| finally: | ||
| # Try to restore original directory, but don't fail if it doesn't exist | ||
| if original_cwd is not None: | ||
| try: | ||
| # Original directory might not exist anymore in Bazel sandbox | ||
| with contextlib.suppress(FileNotFoundError, OSError): | ||
| os.chdir(original_cwd) | ||
| except (FileNotFoundError, OSError): | ||
| # Original directory might not exist anymore in Bazel sandbox | ||
| pass | ||
|
|
||
| return _create_app | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -118,6 +118,7 @@ def sphinx_base_dir(tmp_path_factory: TempPathFactory, pytestconfig) -> Path: | |||||||||
| temp_dir = tmp_path_factory.mktemp("testing_dir") | ||||||||||
| print(f"[blue]Using temporary directory: {temp_dir}[/blue]") | ||||||||||
| return temp_dir | ||||||||||
|
|
||||||||||
| CACHE_DIR.mkdir(parents=True, exist_ok=True) | ||||||||||
| print(f"[green]Using persistent cache directory: {CACHE_DIR}[/green]") | ||||||||||
| return CACHE_DIR | ||||||||||
|
|
@@ -210,11 +211,10 @@ def parse_bazel_output(BR: BuildOutput, pytestconfig) -> BuildOutput: | |||||||||
| split_warnings = [x for x in err_lines if "WARNING: " in x] | ||||||||||
| warning_dict: dict[str, list[str]] = defaultdict(list) | ||||||||||
|
|
||||||||||
| if pytestconfig.get_verbosity() >= 2: | ||||||||||
| if os.getenv("CI"): | ||||||||||
| print("[DEBUG] Raw warnings in CI:") | ||||||||||
| for i, warning in enumerate(split_warnings): | ||||||||||
| print(f"[DEBUG] Warning {i}: {repr(warning)}") | ||||||||||
| if pytestconfig.get_verbosity() >= 2 and os.getenv("CI"): | ||||||||||
| print("[DEBUG] Raw warnings in CI:") | ||||||||||
| for i, warning in enumerate(split_warnings): | ||||||||||
| print(f"[DEBUG] Warning {i}: {repr(warning)}") | ||||||||||
|
|
||||||||||
| for raw_warning in split_warnings: | ||||||||||
| # In the CLI we seem to have some ansi codes in the warnings. | ||||||||||
|
|
@@ -479,46 +479,44 @@ def setup_test_environment(sphinx_base_dir, pytestconfig): | |||||||||
| """Set up the test environment and return necessary paths and metadata.""" | ||||||||||
| git_root = find_git_root() | ||||||||||
| if git_root is None: | ||||||||||
| assert False, "Git root was none" | ||||||||||
| raise RuntimeError("Git root was not found") | ||||||||||
|
Comment on lines
481
to
+482
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit (fix enxt pr):
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||||||||||
|
|
||||||||||
| gh_url = get_github_base_url() | ||||||||||
| current_hash = get_current_git_commit(git_root) | ||||||||||
|
|
||||||||||
| os.chdir(Path(sphinx_base_dir).absolute()) | ||||||||||
|
|
||||||||||
| verbosity = pytestconfig.get_verbosity() | ||||||||||
|
|
||||||||||
| if verbosity >= 2: | ||||||||||
| print(f"[DEBUG] git_root: {git_root}") | ||||||||||
| def debug_print(message): | ||||||||||
| if verbosity >= 2: | ||||||||||
| print(f"[DEBUG] {message}") | ||||||||||
|
|
||||||||||
| # Get GitHub URL and current hash for git override | ||||||||||
| debug_print(f"git_root: {git_root}") | ||||||||||
|
|
||||||||||
| if verbosity >= 2: | ||||||||||
| print(f"[DEBUG] gh_url: {gh_url}") | ||||||||||
| print(f"[DEBUG] current_hash: {current_hash}") | ||||||||||
| print( | ||||||||||
| "[DEBUG] Working directory has uncommitted changes: " | ||||||||||
| f"{has_uncommitted_changes(git_root)}" | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| # Create symlink for local docs-as-code | ||||||||||
| docs_as_code_dest = sphinx_base_dir / "docs_as_code" | ||||||||||
| if docs_as_code_dest.exists() or docs_as_code_dest.is_symlink(): | ||||||||||
| # Remove existing symlink/directory to recreate it | ||||||||||
| if docs_as_code_dest.is_symlink(): | ||||||||||
| docs_as_code_dest.unlink() | ||||||||||
| if verbosity >= 2: | ||||||||||
| print(f"[DEBUG] Removed existing symlink: {docs_as_code_dest}") | ||||||||||
| elif docs_as_code_dest.is_dir(): | ||||||||||
| import shutil | ||||||||||
|
|
||||||||||
| shutil.rmtree(docs_as_code_dest) | ||||||||||
| if verbosity >= 2: | ||||||||||
| print(f"[DEBUG] Removed existing directory: {docs_as_code_dest}") | ||||||||||
|
|
||||||||||
| docs_as_code_dest.symlink_to(git_root) | ||||||||||
| # Get GitHub URL and current hash for git override | ||||||||||
| debug_print(f"gh_url: {gh_url}") | ||||||||||
| debug_print(f"current_hash: {current_hash}") | ||||||||||
| debug_print( | ||||||||||
| "Working directory has uncommitted changes: " | ||||||||||
| f"{has_uncommitted_changes(git_root)}" | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| if verbosity >= 2: | ||||||||||
| print(f"[DEBUG] Symlink created: {docs_as_code_dest} -> {git_root}") | ||||||||||
| def recreate_symlink(dest, target): | ||||||||||
| # Create symlink for local docs-as-code | ||||||||||
| if dest.exists() or dest.is_symlink(): | ||||||||||
| # Remove existing symlink/directory to recreate it | ||||||||||
| if dest.is_symlink(): | ||||||||||
| dest.unlink() | ||||||||||
| debug_print(f"Removed existing symlink: {dest}") | ||||||||||
| elif dest.is_dir(): | ||||||||||
| import shutil | ||||||||||
|
|
||||||||||
| shutil.rmtree(dest) | ||||||||||
| debug_print(f"Removed existing directory: {dest}") | ||||||||||
| dest.symlink_to(target) | ||||||||||
| debug_print(f"Symlink created: {dest} -> {target}") | ||||||||||
|
|
||||||||||
| recreate_symlink(sphinx_base_dir / "docs_as_code", git_root) | ||||||||||
|
|
||||||||||
| return gh_url, current_hash | ||||||||||
|
|
||||||||||
|
|
||||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit fix in next pr:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.