Cleanup makefile and formatting #65
Conversation
WalkthroughThis update introduces a robust, multi-version Python development workflow. It adds a comprehensive setup guide, a cross-platform Makefile with CI-aligned targets, a Bash automation script, and a Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Shell as Terminal/Shell
participant Make as Makefile
participant Pyenv as pyenv
participant Tox as tox
participant Venv as Virtualenv
participant CI as CI Workflow
Dev->>Shell: Run setup-dev-env.sh (macOS)
Shell->>Pyenv: Install Python 3.11, 3.12, 3.13
Shell->>Make: make setup / make dev
Make->>Venv: Create virtualenv, install deps
Shell->>Make: make ci-test / make test-all-versions
Make->>Tox: Run tests across all Python versions
Tox->>Venv: Isolated test environments
Dev->>Make: make lint / make format
Make->>Venv: Run flake8/black
Dev->>CI: Push code, trigger CI
CI->>Tox: Run tests and checks as in local
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 3
🧹 Nitpick comments (14)
ftf_cli/commands/register_output_type.py (1)
89-91: Preserve original exception context with exception chaining
To retain the stack trace and original error context, useraise ... from ewhen re-raising:except ValueError as e: - raise click.UsageError( - f"❌ Error generating lookup tree from properties: {e}" - ) + raise click.UsageError( + f"❌ Error generating lookup tree from properties: {e}" + ) from eftf_cli/utils.py (3)
61-68: Deepifcondition is hard to readEight boolean sub-expressions trigger pylint R0916. Consider early-continuing or extracting a helper:
- if ( - child.data == "block" - and len(child.children) > 2 - ... - ): + if not _is_variable_block(child): + continueWhere
_is_variable_blockhides the gnarly logic.
330-333: Boolean negation can be collapsed- if ( - field_type == "array" - and not override_disable_flag - and not overrides_only_flag - ): + if field_type == "array" and not (override_disable_flag or overrides_only_flag):Reduces cognitive load without changing behaviour.
343-345: Simplify membership test- if not isinstance(pattern_type, str) or ( - pattern_type != "object" and pattern_type != "string" - ): + if not isinstance(pattern_type, str) or pattern_type not in ("object", "string"):Matches pylint R1714 suggestion and reads cleaner.
scripts/setup-dev-env.sh (1)
119-120: Virtual-env path assumption
./env/bin/pippresumesmake setupcreates a venv namedenv/. If the Makefile ever changes that path, the script breaks silently. Prefer reading the.venv_pathfrom the Makefile or usingpython -m pipinside the activated environment.Makefile (3)
1-9: Remove or utilize unused variables
The definitions ofDEFAULT_PYTHON_VERSIONandVENV_DIRaren’t referenced elsewhere. Consider removing them or parameterizing the venv setup to useVENV_DIRand respect the default Python version.
33-33: Review PHONY targets for completeness
The first.PHONYline lists core targets but omitscreate-tox-configand still references a non-existenttest-matrix. Ensure every custom target is declared phony and remove invalid entries.
38-59: Refactor or suppress help target length
The help recipe now prints over 20 lines, triggeringcheckmake’s max-body-length warning. Consider splitting into sub-targets or adding# checkmake: maxbodylength=25to disable it.DEV_SETUP.md (6)
1-4: Expand CI acronym at first mention
Spell out "Continuous Integration (CI)" on first use to aid readers unfamiliar with the abbreviation.
21-35: Include Linux instructions for pyenv installation
Currently the manual setup covers only macOS via Homebrew. Consider adding Linux commands or linking to the official pyenv installation guide to support non-macOS users.
37-47: Ensure Python versions align with Makefile and tox.ini
The hardcoded versions (3.11.9, 3.12.4, 3.13.0) may drift from the Makefile ortox.inisettings. Suggest referencing a single source of truth or instructing users to usemake install-python-versions.
78-92: Consolidate CI-like testing instructions
The "Testing Like CI" section overlaps with "Multi-Version Testing." Consider merging or cross-referencing these sections to avoid duplication and simplify navigation.
109-116: Refine troubleshooting step phrasing
Update "Compare local and CI dependency versions" to "Compare dependency versions between your local environment and CI" for clarity.
117-132: Recommend VS Code extensions and links
Beyond settings, consider adding recommended extensions (e.g., Python, Pylance) and linking to VS Code docs for configuring the workspace.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
DEV_SETUP.md(1 hunks)Makefile(2 hunks)TESTING.md(0 hunks)ftf_cli/commands/add_input.py(1 hunks)ftf_cli/commands/delete_module.py(1 hunks)ftf_cli/commands/generate_module.py(1 hunks)ftf_cli/commands/login.py(3 hunks)ftf_cli/commands/register_output_type.py(2 hunks)ftf_cli/operations.py(2 hunks)ftf_cli/schema.py(1 hunks)ftf_cli/utils.py(10 hunks)scripts/setup-dev-env.sh(1 hunks)tests/commands/test_add_input.py(1 hunks)tests/commands/test_login.py(7 hunks)tests/test_utils.py(12 hunks)tests/test_utils_validation.py(11 hunks)tox.ini(1 hunks)
💤 Files with no reviewable changes (1)
- TESTING.md
🧰 Additional context used
🪛 Ruff (0.11.9)
ftf_cli/commands/delete_module.py
106-108: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
ftf_cli/commands/register_output_type.py
89-91: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🪛 GitHub Actions: Python Tests
tests/commands/test_add_input.py
[error] 175-175: TestAddInputCommand.test_successful_add_input failed: Command exited with code 2 instead of 0. Error encountered while adding input 'db_connection'.
[error] 232-232: TestAddInputCommand.test_not_logged_in_error failed: Expected 'Not logged in' message in output but got error 'Error encountered while adding input test_input' with exit code 2.
[error] 264-264: TestAddInputCommand.test_output_not_found_error failed: Expected 'not found in registered outputs' in output but got error 'Error encountered while adding input test_input' with exit code 2.
[error] 308-308: TestAddInputCommand.test_malformed_properties_fallback failed: Command exited with code 2 instead of 0. Expected success with warning about malformed output properties.
[error] 348-348: TestAddInputCommand.test_missing_properties_fallback failed: Command exited with code 2 instead of 0. Expected success with warning about missing output properties.
[error] 409-409: TestAddInputCommand.test_existing_input_overwrite_warning failed: Command exited with code 2 instead of 0. Expected success with warning on overwriting existing input.
🪛 Pylint (3.3.7)
ftf_cli/utils.py
[refactor] 61-68: Too many boolean expressions in if statement (8/5)
(R0916)
[refactor] 344-344: Consider merging these comparisons with 'in' by using 'pattern_type not in ('object', 'string')'. Use a set instead if elements are hashable.
(R1714)
[refactor] 867-896: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
🪛 LanguageTool
DEV_SETUP.md
[uncategorized] ~114-~114: The preposition ‘to’ seems more likely in this position.
Context: ...ly 2. Check Python version requirements in setup.py 3. Compare local and CI depe...
(AI_HYDRA_LEO_REPLACE_IN_TO)
🪛 checkmake (0.2.2)
Makefile
[warning] 38-38: Target body for "help" exceeds allowed length of 5 (21).
(maxbodylength)
🔇 Additional comments (36)
ftf_cli/commands/delete_module.py (1)
108-108: Ensure newline at EOF
Adding a trailing newline at end-of-file improves POSIX compliance and prevents issues with tools expecting newline-terminated files.ftf_cli/schema.py (1)
109-112: Approve trailing comma addition
This formatting update adds a trailing comma for consistency in multi-line definitions without altering the regex or schema logic.ftf_cli/commands/generate_module.py (1)
77-79: Stylistic change confirmed
Switching to double quotes for the encoding string is purely stylistic and aligns with the project's string-formatting conventions. No functional impact.ftf_cli/operations.py (2)
60-69: Adjust indentation for function parameters
Theregister_moduleparameter list is now indented by 4 spaces (instead of 8), aligning with the function definition and improving readability. No semantic changes.
156-163: Adjust indentation for function parameters
Thepublish_moduleparameters now follow the same 4-space indentation for consistency. No functional impact.tests/commands/test_add_input.py (1)
214-216: Streamline mock context syntax
Consolidating thewith patch(...)invocation into a single line enhances readability without changing test behavior.ftf_cli/commands/register_output_type.py (1)
7-11: Consistent use of multi-line imports for readability
The import statement has been expanded into a parenthesized, multi-line format to improve readability and align with the project's import style.ftf_cli/commands/login.py (3)
32-35: Add trailing comma in multi-lineclick.confirmcall
Including a trailing comma in theclick.confirmcall enhances consistency and avoids diff churn when adding parameters.
42-45: Reformat complex conditional across multiple lines
Splitting the longifcondition into separate lines improves readability without altering the logic.
125-128: Add trailing comma to secondclick.confirminvocation
Consistent use of trailing commas in multi-line calls aligns with the project's formatting guidelines.tests/test_utils_validation.py (8)
3-6: Expand imports into a multi-line block for clarity
Breaking the single-line import into a parenthesized, multi-line form improves readability and parallels other modules.
27-29: Reformat long test assertion into multiple lines
Splitting the assertion string across lines enhances readability without changing the test behavior.
68-68: Add trailing comma in dictionary literal
Ensure consistent trailing commas in dict entries to align with style guidelines.
78-80: Reformattest_pattern_properties_with_yaml_editor_raisesblock
Expanding the multi-line dict literal for improved readability.
96-98: Reformat conflicting UI properties test for readability
Adjusting the dict literal indentation ensures the test remains clear and maintainable.
104-106: Break multi-line assertion into clearer segments
Splitting the assertion across lines makes the expected error message easier to scan.
118-120: Reformat nested properties dict for clarity
Aligning the nested dict structure improves the visual hierarchy in the test.
197-197: Maintain trailing comma consistency in test fixture
Adding a trailing comma in thespecassignment for uniformity.ftf_cli/commands/add_input.py (5)
142-147: Reformat complexifcondition for readability
Splitting the long condition into multiple lines clarifies the check forpropertiesstructure.
153-155: Add trailing comma in output schemas mapping
Including a trailing comma after"interfaces": interfaces_schemaaligns with dict literal conventions.
157-163: Refactor else-block formatting for consistency
Rewriting theelsebranch with multi-lineclick.echoand dict literal enhances readability.
166-168: Reformat exception handler echo for clarity
Expanding the error message printing into a multi-line form improves maintainability.
171-173: Reformat fallback branch for missing properties
Using a consistent multi-line style in theelsebranch matches the surrounding code.tests/commands/test_login.py (1)
1-318: No review needed for formatting-only changes
All modifications in this file are stylistic (string quoting, parentheses, indentation) with no logic or behavior changes.tests/test_utils.py (1)
65-84: Formatting-only changes look solidThe added blank line and trailing-comma adjustments conform to Black/flake8 style without touching test logic.
tox.ini (1)
2-3: Consider droppingpy313for nowPython 3.13 is still a development build; many wheels (including
python-hcl2) are unavailable, sotox -e py313will likely fail.
Either pin to released versions (py311,py312) or mark the job asskip_missing_interpreters = true.scripts/setup-dev-env.sh (1)
96-100:pyenv local 3.11 3.12 3.13may not match installed patch versions
pyenv installpulls e.g.3.11.9, butpyenv local 3.11will fail because that directory doesn’t exist. Capture the exactlatest_versionstrings you installed:pyenv local "${installed_versions[@]}"Makefile (8)
16-18: DRY: Consolidated tool paths for Windows branch
DefiningFLAKE8,BLACK, andTOXvariables for Windows improves maintainability.
24-26: DRY: Consolidated tool paths for Unix branch
Explicitly settingFLAKE8,BLACK, andTOXvariables for Unix-like systems keeps the Makefile consistent.
29-30: Validate Python installation using $(PYTHON)
Great guard against a missing interpreter using$(PYTHON). The error message clearly guides the user to install Python 3.11+.
67-67: Enforce version check before setup
Addingcheck-python-versionas a prerequisite tosetupensures the correct interpreter before creating the virtual environment.
73-73: Upgrade pip consistently after venv creation
Explicitly upgrading pip in both Windows and Unix branches keeps dependencies fresh.Also applies to: 79-79
101-101: Install development tools in dev target
Installingtox,flake8, andblackalongside.[dev]ensures CI tooling is available aftermake dev.Also applies to: 107-107
111-116: Validate venv and run pytest on Windows
The Windows branch correctly checks forpytest.exeand uses$(PYTEST)to run tests. Good error messages guide the user.
117-121: Validate venv and run pytest on Unix-like systems
The Unix branch mirror of the Windows check ensurespytestis present and uses$(PYTEST)consistently.DEV_SETUP.md (1)
5-12: No changes needed in the Quick Setup section.
| pytest>=8.3.5 | ||
| pytest-mock | ||
| pyhcl>=0.4.5 | ||
| commands = python -m pytest tests |
There was a problem hiding this comment.
🛠️ Refactor suggestion
pyhcl will not satisfy the import hcl2 statements
Runtime code imports hcl2, which is provided by the python-hcl2 package, not pyhcl. Installing the wrong dependency will break every test environment.
-deps =
- pytest>=8.3.5
- pytest-mock
- pyhcl>=0.4.5
+deps =
+ pytest>=8.3.5
+ pytest-mock
+ python-hcl2>=4.3.5📝 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.
| pytest>=8.3.5 | |
| pytest-mock | |
| pyhcl>=0.4.5 | |
| commands = python -m pytest tests | |
| deps = | |
| pytest>=8.3.5 | |
| pytest-mock | |
| python-hcl2>=4.3.5 | |
| commands = python -m pytest tests |
🤖 Prompt for AI Agents
In tox.ini around lines 7 to 10, the dependency pyhcl is incorrectly listed but
the runtime code imports hcl2, which requires the python-hcl2 package. Replace
pyhcl>=0.4.5 with python-hcl2 to ensure the correct package is installed and
tests run without import errors.
|
|
||
| with open(os.devnull, "w") as devnull: | ||
| run(["terraform", "fmt", file_path], stdout=devnull, stderr=devnull) | ||
| run(["biuebc", "fmt", file_path], stdout=devnull, stderr=devnull) |
There was a problem hiding this comment.
biuebc fmt looks like a typo — will raise FileNotFoundError
Unless a custom formatter named biuebc is on PATH, this breaks every call to ensure_formatting_for_object.
- run(["biuebc", "fmt", file_path], stdout=devnull, stderr=devnull)
+ run(["terraform", "fmt", file_path], stdout=devnull, stderr=devnull)If you intentionally replaced Terraform’s formatter, document the new requirement in DEV_SETUP.md and CI images.
📝 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.
| run(["biuebc", "fmt", file_path], stdout=devnull, stderr=devnull) | |
| run(["terraform", "fmt", file_path], stdout=devnull, stderr=devnull) |
🤖 Prompt for AI Agents
In ftf_cli/utils.py at line 593, the command 'biuebc fmt' appears to be a typo
and will cause a FileNotFoundError unless 'biuebc' is a valid formatter on PATH.
Replace 'biuebc' with the correct formatter command, likely 'terraform fmt', to
ensure the formatting runs correctly. If 'biuebc' is intentional, add
documentation about this new formatter requirement in DEV_SETUP.md and update CI
images accordingly.
| check-python-version: | ||
| @python3 -c "import sys; exit(0 if sys.version_info >= (3, 11) else 1)" || \ | ||
| (echo "Error: Python 3.11+ required. You have: $$(python3 --version)" && \ | ||
| echo "Please install Python 3.11+ or run 'make install-python-versions'" && exit 1) |
There was a problem hiding this comment.
Ensure cross-platform Python check
The check-python-version target hardcodes python3. Replace these invocations with $(PYTHON) or $(PYTHON_EXE) so it works on Windows too.
🤖 Prompt for AI Agents
In the Makefile at lines 62 to 65, replace all instances of the hardcoded
command `python3` with the variable `$(PYTHON)` or `$(PYTHON_EXE)` to ensure the
Python version check works cross-platform, including on Windows. This involves
updating both the import and version check commands within the
`check-python-version` target to use the variable instead of the fixed `python3`
command.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
ftf_cli/commands/add_input.py (4)
142-147: Flatten the nested dict‐access to improve readability & guardNonecasesThe reformatted multi-line
ifis clearer, but the repeated chained.get()calls still obscure intent and rely on every intermediate value being adict. Consider extractingprops = properties.get("properties")first (with a type check) and then testingpropsfor"attributes"/"interfaces"keys. This shortens the condition, avoids redundant look-ups, and prevents aTypeErrorif"properties"is unexpectedlyNone.
158-163: Minor wording nitYou switched to a multiline
click.echo, which is good for readability. While you’re touching the string, consider enclosing the variable name in back-ticks or quotes to match the earlier warning style ('{output}').
166-168: Consistent error message formattingSame remark as above – quoting
{output}will keep the message formatting consistent with other warnings in this file.
171-173: Consistent warning styleFor uniform UX, wrap
{output}in quotes/back-ticks here as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ftf_cli/commands/add_input.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
ftf_cli/commands/add_input.py
190-190: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (1)
ftf_cli/commands/add_input.py (1)
154-154: No concerns – style-only changeThe trailing comma after
interfaces_schemakeeps future diffs clean.
ftf_cli/commands/add_input.py
Outdated
| raise click.UsageError(f"❌ Error encountered while adding input: {e}") | ||
|
|
There was a problem hiding this comment.
Preserve original exception context
Raising a new click.UsageError without chaining loses the stack-trace and makes debugging harder. Use exception chaining so downstream tooling can still inspect the root cause.
- except Exception as e:
- raise click.UsageError(f"❌ Error encountered while adding input: {e}")
+ except Exception as e:
+ raise click.UsageError(
+ f"❌ Error encountered while adding input: {e}"
+ ) from e📝 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.
| raise click.UsageError(f"❌ Error encountered while adding input: {e}") | |
| except Exception as e: | |
| raise click.UsageError( | |
| f"❌ Error encountered while adding input: {e}" | |
| ) from e |
🧰 Tools
🪛 Ruff (0.11.9)
190-190: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In ftf_cli/commands/add_input.py around lines 190 to 191, the raise statement
for click.UsageError does not preserve the original exception context, which
loses the stack trace. Fix this by using exception chaining with "from e" when
raising the new click.UsageError, so the original exception context is
maintained for better debugging.
Summary by CodeRabbit
New Features
tox.iniconfiguration for managing testing, linting, and formatting environments.Documentation
Refactor
Chores