Refactor config module, restructure docs, improve build #3
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughReplaces Makefile's install-based guard with an init/init.done workflow and adds a public Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as "Developer / User"
participant Make as "Makefile"
participant Guard as "GUARDS/init.done"
participant Config as "iptax.config.base.Configurator"
Dev->>Make: make init / make install
Make->>Config: run init (venv, pyproject checks, config create/validate)
Config-->>Guard: write $(GUARDS)/init.done
Guard-->>Make: signal init complete
Make->>Make: downstream targets require init.done (unit, e2e, lint, format, typecheck, coverage, shell, run, config, report)
Note over Make,Guard: init.done replaces install.done as canonical guard
sequenceDiagram
participant User
participant ConfigBase as "config.base (Configurator)"
participant Wizard as "config.interactive"
User->>ConfigBase: Configurator.create(interactive=True)
ConfigBase->>Wizard: run_interactive_wizard(defaults, list_providers_fn)
Wizard->>User: prompt for employee, product, AI provider, DID, Workday...
Wizard-->>ConfigBase: return populated Settings
ConfigBase->>ConfigBase: validate DID, persist YAML to settings path
ConfigBase-->>User: success (settings saved)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (1)
Makefile (1)
45-56: Clarify intent ofinstalltarget vs. documentation.The renamed
installtarget now performs system/user installation (not dev installation). Ensure documentation and examples clearly distinguish betweenmake init(development) andmake install(production). This is a breaking change for developers following old patterns.Consider adding a comment or deprecation notice in the Makefile clarifying the workflow transition. For example:
# IMPORTANT: Development workflow has changed # - Use 'make init' for development setup (replaces old 'make install') # - Use 'make install' for production system/user installation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Makefile(4 hunks)docs/architecture.md(1 hunks)docs/edge-cases.md(1 hunks)docs/examples.md(1 hunks)docs/implementation.md(1 hunks)docs/requirements.md(1 hunks)docs/testing.md(1 hunks)docs/workflows.md(1 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 46-46: Target body for "install" exceeds allowed length of 5 (10).
(maxbodylength)
[warning] 191-191: Missing required phony target "all"
(minphony)
🪛 LanguageTool
docs/implementation.md
[uncategorized] ~109-~109: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...ight) with fallback 2. Report compiler (markdown formatting) 3. PDF generation (WeasyPri...
(MARKDOWN_NNP)
[uncategorized] ~279-~279: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...lates, iterate - Fallback: Focus on markdown first, PDFs later ### Schedule Risks ...
(MARKDOWN_NNP)
docs/workflows.md
[style] ~3-~3: This phrase is redundant (‘I’ stands for ‘interface’). Use simply “CLI”.
Context: ...nt describes the detailed workflows and CLI interface design for the iptax tool. **See also:...
(ACRONYM_TAUTOLOGY)
[style] ~132-~132: This phrase is redundant (‘I’ stands for ‘Interface’). Use simply “CLIInterface”.
Context: ...al summary with next steps --- ## CLI Interface Design ### Command Structure ipta...
(ACRONYM_TAUTOLOGY)
docs/edge-cases.md
[grammar] ~3-~3: Use a hyphen to join words.
Context: ... document describes edge cases and error handling strategies for the iptax tool. ...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
docs/implementation.md
135-135: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/workflows.md
22-22: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
46-46: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
68-68: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
81-81: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
96-96: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
113-113: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
123-123: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
136-136: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
157-157: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
207-207: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
231-231: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
294-294: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
313-313: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
332-332: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
351-351: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/edge-cases.md
43-43: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/testing.md
68-68: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
287-287: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/requirements.md
90-90: Bare URL used
(MD034, no-bare-urls)
183-183: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
200-200: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/architecture.md
43-43: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/examples.md
366-366: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
382-382: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
401-401: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
417-417: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
528-528: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
555-555: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (9)
Makefile (2)
67-74: Verify init target guard chain is complete.The
inittarget depends on$(GUARDS)/init.done, which is created by the recipe. Confirm all downstream targets (unit, e2e, lint, format, typecheck, coverage, shell, run, config, report) correctly cascade throughinit.doneand that no target can bypass this initialization. All references appear correct.
112-114: Format target should explicitly depend on init.done.Line 112 marks
formatas.PHONYbut the target itself does not contain a prerequisite guard. The actual guard enforcement happens at line 120 ($(GUARDS)/format.done). This is correct (format runs immediately when invoked), but for consistency with lint/typecheck, consider documenting this pattern.docs/implementation.md (1)
1-13: Cross-reference navigation is well-structured.The documentation provides a clear "See also" section linking to related documentation files. This improves discoverability across the split documentation.
docs/edge-cases.md (1)
1-13: Cross-reference structure is comprehensive.The navigation section clearly links to related documentation, supporting modular reference patterns. Well organized.
docs/examples.md (1)
441-441: Verify make targets align with Makefile changes.Examples reference
make install(line 441) for installation. With the Makefile changes in this PR, update references:
- Line 441:
make install→make initfor development setup- Line 656: Documentation shows
make initwhich aligns with new workflowVerify all command examples use the correct targets.
Also applies to: 656-656
docs/workflows.md (2)
1-13: Documentation navigation structure is excellent.Cross-references to related documentation are well-organized and consistent. This supports users navigating the documentation suite.
151-197: Verify command examples match CLI implementation.Examples show command syntax and options. Verify these align with the planned Click CLI implementation. For instance, the
--month YYYY-MMformat,--skip-ai,--skip-workdayflags should be confirmed in the actual CLI code.docs/requirements.md (1)
95-103: I need to search with more specific information. Let me try a refined query with the full GitHub URL.Verify did SDK version requirement and assess the need for updating the PR reference.
The web search could not definitively determine whether psss/did PR #311 has been merged to main. However, based on the retrieved learnings, this temporary PR reference is an accepted practice in the iptax project because the project maintainer (cardil) owns and maintains that PR until it's merged.
Since the current knowledge cutoff is March 2025 and today is November 25, 2025, the PR status may have changed. You should verify directly by:
- Checking the PR status at psss/did#311
- If merged, update
pyproject.tomlto reference the stable release or the main branch instead of the PR reference- If still open, document the reason for the temporary reference in a comment
docs/testing.md (1)
1-50: Well-structured testing documentation with clear examples and cross-references.The document effectively communicates testing strategy, Make-based orchestration, and the new init-based guard-file system. Cross-references to related documentation (lines 5–12) improve discoverability, and the practical examples for test cases and mocks provide good guidance for developers.
cardil
force-pushed
the
fix/housekeeping
branch
from
a8a1667 to
5db45fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/iptax/models.py (1)
334-357:from_yaml_filedocstring and behavior disagree on error types.The docstring says this method raises
TypeErrorwhen “the YAML is invalid or validation fails”, but the implementation only raises:
FileNotFoundErrorexplicitly, andTypeErrorwhen the loaded data is not a mapping,while:
- YAML syntax errors will bubble up as
yaml.YAMLError, and- Pydantic validation failures will raise
pydantic.ValidationError.That mismatch can surprise callers and contradicts the PR’s stated intention that YAML-invalid or structurally-wrong configs become
TypeErrors.Consider either:
- Updating behavior to match the docstring, e.g.:
+from pydantic import ValidationError +import yaml ... if not path.exists(): raise FileNotFoundError(f"Settings file not found: {path}") - with path.open(encoding="utf-8") as f: - data = yaml.safe_load(f) + try: + with path.open(encoding="utf-8") as f: + data = yaml.safe_load(f) + except yaml.YAMLError as exc: + raise TypeError(f"Invalid YAML in settings file {path}: {exc}") from exc if not isinstance(data, dict): raise TypeError(f"Invalid settings file format in {path}: expected a mapping") - return cls(**data) + try: + return cls(**data) + except ValidationError as exc: + raise TypeError(f"Settings validation failed for {path}: {exc}") from excor
- Relaxing the docstring to list the actual exceptions (
yaml.YAMLError,ValidationError) and only reserveTypeErrorfor the non-mapping case.I’d recommend aligning behavior and tests with whichever contract you prefer, but they should be consistent.
♻️ Duplicate comments (10)
docs/workflows.md (2)
20-42: Add language identifiers to workflow and TUI/output fenced blocks.Several fenced blocks that represent step lists, sample history tables, or TUI screens still use bare ``` fences (e.g., the blocks starting at Lines 22, 46, 68, 81, 96, 113, 123, 231, 313, 332, 351). To satisfy MD040 and improve readability, please annotate them with
text, for example:-``` +```text 1.1 Load configuration from ~/.config/iptax/settings.yaml ...Apply the same pattern to other non-code/output blocks (history table, “Changes Review” list view, detail view, etc.), while keeping actual shell examples as
bash.Also applies to: 44-128, 230-324, 332-374
1-4: Simplify “CLI interface design” wording.Two occurrences still use the redundant phrase “CLI interface”:
- Line 3: “CLI interface design”
- Line 132: “## CLI Interface Design”
Please shorten to “CLI design” (e.g., “CLI design” in the intro sentence and “## CLI Design” as the heading) to avoid the tautology flagged by LanguageTool.
Also applies to: 132-136
docs/testing.md (3)
283-293: Add language identifier to the fixtures tree block.The fixtures directory tree at Lines 287–293 is fenced with a bare ``` and trips MD040. Since this is file-structure text, annotating it as
textkeeps linting happy:-``` +```text tests/fixtures/ ├── sample_did_output.py # Mock did SDK responses ...
301-325: Usemake initinstead ofmake installin the Make-based test section.Under “Using Make (Recommended)”, the snippet still instructs:
# Install dependencies make installGiven the Makefile now distinguishes dev setup (
init) from production install (install), this should be:-# Install dependencies -make install +# Initialize development environment +make initso contributors follow the correct init-based workflow.
437-456: Update CI example to initialize withmake init.The GitHub Actions example still uses
make installin the “Install dependencies” step. With the new workflow, CI should callmake initto create the venv and install dev extras:- - name: Install dependencies - run: make install + - name: Initialize dev environment + run: make initso CI and local workflows stay consistent.
docs/architecture.md (2)
28-40: Core dependencies list is still incomplete vspyproject.toml.The "Key Dependencies" section omits several packages that are part of the core runtime in
pyproject.toml(e.g.,python-dateutil,markdown,questionary, and thedidVCS dependency). Please extend this list so it reflects all core pieces, and explicitly call out thedidGit dependency (with its URL and pinned ref) while keeping the note thatpyproject.tomlis the source of truth.Based on learnings, this should include the pinned
didGit dependency.
41-80: Add a language identifier and fix project tree inaccuracies.Two issues in the project structure block:
- The fenced block at Line 43 lacks a language identifier, which triggers MD040. For a directory tree,
textis appropriate:-``` +```text iptax-reporter/
- The tree content is out of sync with the codebase and comments:
src/iptax/config.pyshould reflect the newsrc/iptax/config/package layout (e.g.,config/base.py,config/interactive.py).- Under
docs/, the commentrequirements.md # This fileis wrong inarchitecture.md; that comment belongs onarchitecture.md.Consider updating this block to mirror the actual layout (and new config package) so contributors are not misled.
docs/edge-cases.md (1)
30-315: Addbashlanguage identifiers to shell/error code fences.Many fenced blocks that show shell errors or commands (e.g., at Lines 35, 58, 75, 107, 119, 139, 156, 178, 200, 249, 265, 283, 303, 325, 347, 367, 387, 408, 430, 484, 499, and similar later blocks) still use plain
without a language. To satisfy MD040 and improve readability, please change those opening fences to `bashwhile leaving Python snippets aspython` and data examples (JSON/TOML) in their current languages.docs/implementation.md (1)
107-121: Capitalize “Markdown” when referring to the format.Two places still use lowercase “markdown” as a format name:
- Line 109:
Report compiler (markdown formatting)- Line 279:
Focus on markdown first, PDFs laterPlease change these to “Markdown formatting” and “Markdown first” to reflect the proper noun usage.
Also applies to: 277-280
pyproject.toml (1)
146-151: Sensible ignore rules for the project context.
S101: Appropriate since pytest usesassertextensivelyTRY003: Appropriate for CLI tools where user-facing error messages need to be descriptive inlineThe previous review concern about T20 violation appears resolved - the codebase now uses
questionary.print()(a library method) rather than the builtinprint(), which shouldn't trigger T20.
🧹 Nitpick comments (11)
docs/edge-cases.md (1)
592-601: Check whether “Fresh Install” should now referencemake init.The recovery step
Fresh Install: make clean installpredates the newinit/installsplit. Giveninstallis now aimed at system/user installs andinitis the dev-environment bootstrap, please verify whether this line should instead point tomake clean init(for developers) or stay asmake clean install(for end users), and adjust wording accordingly so the distinction is clear.docs/implementation.md (1)
135-156: Add language identifier to the timeline fenced block.The timeline/milestones block starting at Line 135 uses a bare ``` fence and triggers MD040. Since this is prose-like timeline text, marking it as
textkeeps it clear and lints cleanly:-``` +```text Week 1-2: Phase 1 - Foundation ...src/iptax/models.py (2)
92-98: MAX_PERCENTAGE usage looks good; TRY003 warning is ignorable here.The switch to
MAX_PERCENTAGEin thecreative_work_percentagevalidator is a nice improvement over the hard-coded100. Ruff’s TRY003 warning about the longer error message is stylistic; in this domain-specific error, the explicit message is acceptable and helps users understand misconfiguration. No changes needed unless you want to conform strictly to TRY003 by introducing a dedicated exception class.
359-380:to_yaml_filebehavior is solid; note chmod portability only if targeting Windows.The updated
to_yaml_fileimplementation (creating parent dirs, writing UTF‑8 withsafe_dump, and then forcing0o600permissions) is a good hardening of config handling and aligns with the security docs. Be aware thatpath.chmod(0o600)is a no-op or partially honored on some non-POSIX platforms; if you intend full cross-platform support, you may eventually want platform-conditional handling, but it’s fine for now.Makefile (1)
67-75:init-guarded workflow and test/quality targets look consistent.The
inittarget, backed by.make/init.doneand.make/venv.done, and the updated dependencies forunit.passed,e2e.passed,lint.passed,format.done,typecheck.passed,coverage,shell,run,config, andreportcreate a coherent init‑first workflow:
- Venv and dev deps are only set up once unless
pyproject.tomlchanges.- All heavyweight operations (tests, lint, formatting, typecheck, coverage, CLI helpers) depend on having the dev env initialized.
This matches the documented guard‑file semantics and should make local and CI runs predictable.
Also applies to: 84-96, 106-121, 129-135, 171-193
src/iptax/config/interactive.py (3)
300-316: Unuseddid_config_pathparameter - potential oversight.The
did_config_pathparameter is passed but never used. The default value shown to the user comes fromdefaults.did.config_pathor the model field default (line 312), not from the passed parameter. This appears to be an oversight.Consider using
did_config_pathas the fallback default:# Get default from model or existing config if defaults: default_did_path = str(defaults.did.config_path) else: - default_did_path = DidConfig.model_fields["config_path"].default + default_did_path = str(did_config_path)
113-115: Validation logic has edge case with empty string.The validation
x.isdigit()returnsFalsefor empty strings, which is correct, but the error message could be confusing since an empty input would show "Must be 0-100" rather than indicating it's required.This is minor since the default is pre-populated, but consider:
- validate=lambda x: (x.isdigit() and 0 <= int(x) <= MAX_PERCENTAGE) - or f"Must be 0-{MAX_PERCENTAGE}", + validate=lambda x: (x.strip().isdigit() and 0 <= int(x) <= MAX_PERCENTAGE) + or f"Please enter a number between 0 and {MAX_PERCENTAGE}",
337-339: Silent fallback when no providers selected may surprise users.When the user selects zero providers, the code silently falls back to all available providers with just a warning print. Consider whether this should be a hard requirement that loops until valid input:
The current behavior is acceptable for UX (prevents user from getting stuck), but the warning style could be more visible:
if not selected_providers: - questionary.print("⚠ At least one provider must be selected", style="bold red") + questionary.print("⚠ No providers selected - using all available providers", style="bold yellow") selected_providers = available_providerssrc/iptax/config/base.py (3)
125-137: Redundant FileNotFoundError handling.The
FileNotFoundErrorcatch on lines 133-137 is unreachable because line 125 already checksif not self.settings_path.exists()and raises before callingSettings.from_yaml_file(). Consider removing the redundant handler:try: return Settings.from_yaml_file(self.settings_path) - except FileNotFoundError as e: - raise ConfigError( - f"Configuration file not found: {self.settings_path}\n\n" - "Please run 'iptax config' to set up your configuration." - ) from e except yaml.YAMLError as e:Alternatively, keep it as defensive coding against TOCTOU race conditions where the file could be deleted between the exists check and the read.
287-293: Temporary state mutation pattern is fragile.The
_list_providers_for_pathmethod temporarily mutatesself.did_config_path, which could cause issues if exceptions occur or in concurrent scenarios. Consider a cleaner approach:def _list_providers_for_path(self, path: Path) -> list[str]: - """List providers for a given did config path. - - Helper method to provide a compatible interface for the interactive wizard. - - Args: - path: Path to did config file - - Returns: - List of provider names - """ - original_path = self.did_config_path - try: - self.did_config_path = path - return self.list_did_providers() - finally: - self.did_config_path = original_path + """List providers for a given did config path.""" + temp_configurator = Configurator(did_config_path=path) + return temp_configurator.list_did_providers()This avoids mutation and is more thread-safe if ever needed.
319-323: Silent fallback to hardcoded provider on error.When
list_did_providers()fails, the code silently falls back to["github.com"]. This could mask configuration issues. Consider logging a warning:try: available_providers = self.list_did_providers() except DidConfigError: + # Fallback to default provider if did config is not fully configured available_providers = ["github.com"]Or re-raise since
validate_did_config()was already called at line 254, which should have caught missing configs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Makefile(4 hunks)docs/architecture.md(1 hunks)docs/edge-cases.md(1 hunks)docs/examples.md(1 hunks)docs/implementation.md(1 hunks)docs/requirements.md(1 hunks)docs/testing.md(1 hunks)docs/workflows.md(1 hunks)pyproject.toml(1 hunks)src/iptax/config.py(0 hunks)src/iptax/config/__init__.py(1 hunks)src/iptax/config/base.py(1 hunks)src/iptax/config/interactive.py(1 hunks)src/iptax/models.py(6 hunks)tests/unit/test_config.py(5 hunks)
💤 Files with no reviewable changes (1)
- src/iptax/config.py
✅ Files skipped from review due to trivial changes (1)
- docs/requirements.md
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: cardil
Repo: cardil/iptax PR: 2
File: pyproject.toml:59-61
Timestamp: 2025-11-25T12:20:43.221Z
Learning: In the iptax project (pyproject.toml), the "did" dependency is pinned to refs/pull/311/head because the project maintainer (cardil) owns and maintains that PR until it's merged, making it an acceptable temporary reference.
🧬 Code graph analysis (2)
src/iptax/config/base.py (3)
src/iptax/cli.py (2)
config(41-98)report(28-34)src/iptax/config/interactive.py (1)
run_interactive_wizard(30-62)src/iptax/models.py (10)
DidConfig(252-298)DisabledAIConfig(207-214)EmployeeInfo(36-50)ProductConfig(63-73)ReportConfig(76-110)Settings(301-379)WorkdayConfig(224-249)from_yaml_file(335-357)to_yaml_file(359-379)get_config_path(292-298)
src/iptax/config/__init__.py (1)
src/iptax/config/base.py (11)
ConfigError(29-32)Configurator(41-340)DidConfigError(35-38)create_default_config(412-427)get_config_path(362-368)get_did_config_path(371-377)list_did_providers(196-236)list_did_providers(396-409)load_settings(346-359)validate_did_config(149-178)validate_did_config(380-393)
🪛 checkmake (0.2.2)
Makefile
[warning] 46-46: Target body for "install" exceeds allowed length of 5 (10).
(maxbodylength)
[warning] 191-191: Missing required phony target "all"
(minphony)
🪛 LanguageTool
docs/implementation.md
[uncategorized] ~109-~109: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...ight) with fallback 2. Report compiler (markdown formatting) 3. PDF generation (WeasyPri...
(MARKDOWN_NNP)
[uncategorized] ~279-~279: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...lates, iterate - Fallback: Focus on markdown first, PDFs later ### Schedule Risks ...
(MARKDOWN_NNP)
docs/workflows.md
[style] ~3-~3: This phrase is redundant (‘I’ stands for ‘interface’). Use simply “CLI”.
Context: ...nt describes the detailed workflows and CLI interface design for the iptax tool. **See also:...
(ACRONYM_TAUTOLOGY)
[style] ~132-~132: This phrase is redundant (‘I’ stands for ‘Interface’). Use simply “CLIInterface”.
Context: ...al summary with next steps --- ## CLI Interface Design ### Command Structure ipta...
(ACRONYM_TAUTOLOGY)
🪛 markdownlint-cli2 (0.18.1)
docs/testing.md
90-90: Bare URL used
(MD034, no-bare-urls)
183-183: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
200-200: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/examples.md
68-68: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
287-287: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/implementation.md
68-68: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
287-287: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/architecture.md
43-43: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/edge-cases.md
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
58-58: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
75-75: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
107-107: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
119-119: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
139-139: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
156-156: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
178-178: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
200-200: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
249-249: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
265-265: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
283-283: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
303-303: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
325-325: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
347-347: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
367-367: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
387-387: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
408-408: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
430-430: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
484-484: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
499-499: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/workflows.md
22-22: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
46-46: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
68-68: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
81-81: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
96-96: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
113-113: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
123-123: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
136-136: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
157-157: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
207-207: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
231-231: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
294-294: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
313-313: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
332-332: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
351-351: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.5)
src/iptax/config/interactive.py
302-302: Unused function argument: did_config_path
(ARG001)
src/iptax/models.py
97-97: Avoid specifying long messages outside the exception class
(TRY003)
349-349: Avoid specifying long messages outside the exception class
(TRY003)
355-355: Avoid specifying long messages outside the exception class
(TRY003)
src/iptax/config/base.py
126-129: Avoid specifying long messages outside the exception class
(TRY003)
134-137: Avoid specifying long messages outside the exception class
(TRY003)
139-142: Avoid specifying long messages outside the exception class
(TRY003)
144-147: Avoid specifying long messages outside the exception class
(TRY003)
163-168: Avoid specifying long messages outside the exception class
(TRY003)
171-171: Avoid specifying long messages outside the exception class
(TRY003)
174-176: Avoid specifying long messages outside the exception class
(TRY003)
186-194: Avoid specifying long messages outside the exception class
(TRY003)
233-236: Avoid specifying long messages outside the exception class
(TRY003)
275-275: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (14)
src/iptax/models.py (1)
498-513: Simplified GitLab URL logic is fine.The removal of the redundant
elsebranch and unconditional GitLab-style URL construction for non‑GitHub providers keepsChange.get_urlclearer without changing behavior (GitHub still returns/pull/{number}, everything else uses/-/merge_requests/{number}). No further changes needed.Makefile (1)
45-56:installtarget behavior matches the new “production install” role.The new
installtarget correctly installsiptaxinto the system or user environment using the globalpython3 -m pip. This cleanly separates it from the venv‑basedinitflow and aligns with the PR’s goal of distinguishing developer vs. end‑user installs. No changes needed here.tests/unit/test_config.py (3)
467-471: Mock paths correctly updated for new module structure.The patches now target
iptax.config.interactive.questionarywhich aligns with the refactored module structure where the interactive wizard lives insrc/iptax/config/interactive.py.
474-482: Type annotations added to test helpers.The
text_side_effecthelper now has explicit type annotations. Usingobjectfor**kwargsis acceptable here since the mock doesn't need to type-check the keyword arguments.
487-494: Consistent type annotations for confirm helper.Same pattern applied to
confirm_side_effect- good consistency with the text helper above.pyproject.toml (2)
125-145: Comprehensive linting rule expansion.The addition of security checks (S/flake8-bandit), type annotations (ANN), exception handling (TRY), and other categories strengthens code quality. The selection is well-balanced for a CLI application.
153-160: Well-considered per-file ignores for test code.The test-specific ignores are appropriate:
ANN001/ANN201: pytest fixtures and test functions don't benefit from strict annotationsPLR2004: Magic values in assertions are common and readable in testsPLC0415: Lazy imports in tests are acceptable for test isolationsrc/iptax/config/__init__.py (1)
1-29: Clean package interface with explicit public API.The
__init__.pyproperly re-exports all public entities frombase.pywith an explicit__all__list. This pattern:
- Provides a stable public API
- Allows internal refactoring without breaking imports
- Makes the public interface discoverable
src/iptax/config/interactive.py (2)
136-137: Safe access pattern for defaults.ai.The condition
if defaultsis checked before accessingdefaults.ai, which is correct. Theisinstancecheck handles the case properly.
30-62: Well-structured wizard orchestration.The
run_interactive_wizardfunction cleanly orchestrates the configuration flow by delegating to focused helper functions. The separation of concerns makes the code maintainable and testable.src/iptax/config/base.py (4)
230-236: Broad exception catch is appropriate here.The broad
except Exceptionis intentional to catch any parsing errors from thedidSDK, with proper re-raising ofDidConfigErrorinstances. The pattern correctly wraps unknown errors with context.
41-58: Well-documented Configurator class.The class docstring provides clear examples showing both default and custom path usage patterns. This is helpful for testing and production scenarios.
81-114: Proper XDG and HOME environment variable handling.The path resolution correctly respects
XDG_CONFIG_HOMEfor settings andHOMEfor both paths, with appropriate fallbacks toPath.home(). This follows Linux/Unix conventions.
343-427: Clean convenience function API.The module-level convenience functions (
load_settings,get_config_path, etc.) provide a simple API for common use cases while theConfiguratorclass remains available for advanced scenarios requiring custom paths.
| ### Typical Project Structure | ||
|
|
||
| ``` | ||
| ~/.config/iptax/ | ||
| └── settings.yaml | ||
| ~/.cache/iptax/ | ||
| ├── history.toml | ||
| └── ai_cache.json | ||
| ~/.did/ | ||
| └── config | ||
| ~/Documents/iptax/ | ||
| ├── 2024/ | ||
| │ ├── 2024-10 IP TAX Report.md | ||
| │ ├── 2024-10 IP TAX Work Card.pdf | ||
| │ ├── 2024-10 IP TAX Raport.pdf | ||
| │ ├── 2024-11 IP TAX Report.md | ||
| │ ├── 2024-11 IP TAX Work Card.pdf | ||
| │ └── 2024-11 IP TAX Raport.pdf | ||
| └── 2025/ | ||
| ├── 2025-01 IP TAX Report.md | ||
| ├── 2025-01 IP TAX Work Card.pdf | ||
| └── 2025-01 IP TAX Raport.pdf | ||
| ``` | ||
|
|
||
| ### Development Project Structure | ||
|
|
||
| ``` | ||
| iptax-reporter/ | ||
| ├── .git/ | ||
| ├── .gitignore | ||
| ├── .make/ # Guard files | ||
| │ ├── install.done | ||
| │ ├── unit.passed | ||
| │ └── e2e.passed | ||
| ├── src/ | ||
| │ └── iptax/ | ||
| │ ├── __init__.py | ||
| │ ├── cli.py | ||
| │ ├── config.py | ||
| │ └── ... | ||
| ├── tests/ | ||
| │ ├── unit/ | ||
| │ ├── e2e/ | ||
| │ └── fixtures/ | ||
| ├── docs/ | ||
| │ ├── project.md | ||
| │ ├── requirements.md | ||
| │ ├── architecture.md | ||
| │ ├── workflows.md | ||
| │ ├── testing.md | ||
| │ ├── implementation.md | ||
| │ ├── edge-cases.md | ||
| │ └── examples.md | ||
| ├── pyproject.toml | ||
| ├── Makefile | ||
| ├── README.md | ||
| └── LICENSE | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers and refresh dev project structure details.
In the directory-structure examples:
- The blocks at Lines 528–551 and 555–586 use bare ``` fences. To satisfy MD040 and clarify intent, please tag them as plain text:
-```
+```text
~/.config/iptax/
...- The “Development Project Structure” tree still shows:
.make/install.doneinstead of.make/init.done(and omits other guards likevenv.done,lint.passed, etc.).src/iptax/config.pyinstead of the newsrc/iptax/config/package.
Consider updating this tree to match the current Makefile guard files and the new config package layout.
🤖 Prompt for AI Agents
In docs/examples.md around lines 526–586, two fenced directory examples (lines
~528–551 and ~555–586) are missing language identifiers and should be marked as
plain text by changing ``` to ```text; additionally update the "Development
Project Structure" tree to reflect current layout by renaming .make/install.done
to .make/init.done, adding other guard files (e.g. venv.done, lint.passed,
unit.passed, e2e.passed as appropriate), and change src/iptax/config.py to the
new package layout src/iptax/config/ (and list its __init__.py or submodules as
needed) so the example matches the Makefile guards and package refactor.
Split config.py into organized package, split monolithic docs into focused files, clarify Makefile targets, enhance linting. **Code Refactoring:** - Split src/iptax/config.py → config/ package (base.py, interactive.py) - Separated Configurator class logic from interactive wizard - Updated imports and tests **Documentation:** - Split project.md → focused docs (architecture, requirements, workflows, testing, implementation, edge-cases, examples) - Updated project.md as navigation index **Build:** - Renamed `make install` → `make init` (dev setup) - New `make install` for production - Added comprehensive ruff linting rules Assisted-by: 🤖 Claude <noreply@anthropic.com>
cardil
force-pushed
the
fix/housekeeping
branch
from
5db45fa to
fdb5550
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Makefile(4 hunks)docs/architecture.md(1 hunks)docs/edge-cases.md(1 hunks)docs/examples.md(1 hunks)docs/implementation.md(1 hunks)docs/requirements.md(1 hunks)docs/testing.md(1 hunks)docs/workflows.md(1 hunks)pyproject.toml(1 hunks)src/iptax/config.py(0 hunks)src/iptax/config/__init__.py(1 hunks)src/iptax/config/base.py(1 hunks)src/iptax/config/interactive.py(1 hunks)src/iptax/models.py(6 hunks)tests/unit/test_config.py(5 hunks)
💤 Files with no reviewable changes (1)
- src/iptax/config.py
🚧 Files skipped from review as they are similar to previous changes (7)
- src/iptax/config/init.py
- docs/architecture.md
- docs/workflows.md
- docs/implementation.md
- docs/edge-cases.md
- docs/requirements.md
- docs/testing.md
🧰 Additional context used
🧬 Code graph analysis (2)
src/iptax/config/interactive.py (1)
src/iptax/models.py (9)
DidConfig(252-298)DisabledAIConfig(207-214)EmployeeInfo(36-50)GeminiProviderConfig(138-168)ProductConfig(63-73)ReportConfig(76-110)Settings(301-379)VertexAIProviderConfig(171-204)WorkdayConfig(224-249)
src/iptax/config/base.py (2)
src/iptax/config/interactive.py (1)
run_interactive_wizard(30-62)src/iptax/models.py (10)
DidConfig(252-298)DisabledAIConfig(207-214)EmployeeInfo(36-50)ProductConfig(63-73)ReportConfig(76-110)Settings(301-379)WorkdayConfig(224-249)from_yaml_file(335-357)to_yaml_file(359-379)get_config_path(292-298)
🪛 checkmake (0.2.2)
Makefile
[warning] 46-46: Target body for "install" exceeds allowed length of 5 (10).
(maxbodylength)
[warning] 191-191: Missing required phony target "all"
(minphony)
🪛 markdownlint-cli2 (0.18.1)
docs/examples.md
366-366: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
382-382: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
401-401: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
417-417: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.5)
src/iptax/config/interactive.py
302-302: Unused function argument: did_config_path
(ARG001)
src/iptax/config/base.py
126-129: Avoid specifying long messages outside the exception class
(TRY003)
134-137: Avoid specifying long messages outside the exception class
(TRY003)
139-142: Avoid specifying long messages outside the exception class
(TRY003)
144-147: Avoid specifying long messages outside the exception class
(TRY003)
163-168: Avoid specifying long messages outside the exception class
(TRY003)
171-171: Avoid specifying long messages outside the exception class
(TRY003)
174-176: Avoid specifying long messages outside the exception class
(TRY003)
186-194: Avoid specifying long messages outside the exception class
(TRY003)
233-236: Avoid specifying long messages outside the exception class
(TRY003)
275-275: Avoid specifying long messages outside the exception class
(TRY003)
src/iptax/models.py
97-97: Avoid specifying long messages outside the exception class
(TRY003)
349-349: Avoid specifying long messages outside the exception class
(TRY003)
355-355: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (22)
pyproject.toml (1)
125-161: LGTM! Well-configured linting rules.The expanded ruff configuration is comprehensive and well-thought-out:
- Security checks (S), exception handling best practices (TRY), and code simplification (SIM) improve code quality
- Type annotation enforcement (ANN) with test-specific exemptions is appropriate
- The ignore list correctly excludes S101 (pytest uses assert) and TRY003 (descriptive exception messages are valuable)
- Per-file ignores for tests are well-justified
src/iptax/models.py (4)
11-11: LGTM! Good addition of constant.The
yamlimport andMAX_PERCENTAGEconstant improve code maintainability and readability.Also applies to: 22-24
346-356: LGTM! More accurate exception type.Changing from
ValueErrortoTypeErrorfor invalid YAML data is more semantically accurate, as the issue is with the data type (not a mapping) rather than just an invalid value.
369-379: LGTM! Improved YAML handling.Using
path.open()with a context manager and explicit encoding is best practice. The 600 permissions for the config file properly protect potentially sensitive configuration data.
512-513: LGTM! Cleaner GitLab URL construction.Removing the redundant
elsebranch simplifies the code without changing behavior.tests/unit/test_config.py (2)
467-470: LGTM! Correct alignment with config refactor.The patch targets have been updated to
iptax.config.interactive.questionary.*to match the new module structure where interactive configuration has been separated into its own module.Also applies to: 525-529, 583-587, 643-646
474-474: LGTM! Improved type safety in tests.Adding explicit type annotations to the side-effect functions (
text_side_effectandconfirm_side_effect) improves test clarity and type checking.Also applies to: 487-487
src/iptax/config/interactive.py (3)
30-62: LGTM! Well-structured wizard orchestration.The
run_interactive_wizardfunction provides a clean, modular entry point for interactive configuration. The separation into dedicated helper functions for each configuration section makes the code easy to follow and maintain.
65-130: LGTM! Comprehensive input validation.The helper functions provide good validation for user inputs:
- Non-empty checks for required fields
- Percentage bounds checking using MAX_PERCENTAGE
- Clear prompts with defaults
300-348: LGTM! Effective did configuration flow.The
_get_did_configfunction handles provider selection well:
- Reads available providers dynamically
- Pre-selects providers based on defaults
- Falls back to all providers if none selected
- Provides clear visual feedback
Note: The
did_config_pathparameter is used indirectly via thelist_providers_fncallback (line 320), so the static analysis warning is a false positive.Makefile (3)
45-56: LGTM! Clear separation of production vs development workflows.The new
installtarget properly distinguishes between system-wide installation (for end users) and development environment setup. The logic to detect write permissions and fall back to user installation is robust.
67-74: LGTM! Renamed init target clarifies developer workflow.Renaming the development setup target from
installtoinitbetter communicates its purpose as environment initialization rather than production installation. The guard file naming (init.done) is now consistent with the target name.
84-84: LGTM! Consistent dependency updates throughout.All development-related targets have been correctly updated to depend on
$(GUARDS)/init.doneinstead of the oldinstall.doneguard file, maintaining consistency across the entire Makefile.Also applies to: 92-92, 98-98, 106-106, 112-112, 120-120, 131-131, 172-172, 180-180, 184-184, 188-188, 192-192
src/iptax/config/base.py (9)
29-38: LGTM! Clear exception hierarchy.The exception hierarchy (
ConfigErroras base,DidConfigErroras specialized) provides good separation of concerns and enables targeted error handling.
60-80: LGTM! Flexible constructor design.The
Configuratorconstructor allows custom paths (useful for testing) while defaulting to standard locations. This mirrors the design pattern used by the did SDK itself.
82-114: LGTM! XDG Base Directory compliance.The default path resolution properly respects
XDG_CONFIG_HOMEandHOMEenvironment variables, following the XDG Base Directory Specification for configuration files.
116-148: LGTM! Comprehensive error handling in load().The
load()method provides excellent error messages with actionable guidance:
- Clear indication of what failed
- Suggestions for how to fix (run
iptax config)- Proper exception chaining
149-178: LGTM! Thorough did config validation.The
validate_did_config()method checks existence, file type, and read permissions, providing specific error messages for each failure mode.
196-236: LGTM! Robust provider listing with fallback.The
list_did_providers()method:
- Uses the did SDK for proper config parsing
- Filters out the
[general]section- Provides helpful error messages with examples
- Catches and wraps exceptions appropriately
238-276: LGTM! Well-structured config creation flow.The
create()method:
- Validates prerequisites (did config)
- Creates directories as needed
- Loads existing settings for defaults (suppresses errors gracefully)
- Delegates to appropriate wizard or template creation
- Handles save errors with clear messages
295-340: LGTM! Clean wizard integration and fallback template.The
_interactive_config_wizard()delegates to the separate interactive module, maintaining separation of concerns. The_create_minimal_config()provides a sensible fallback with placeholder values when interactive mode is unavailable.
346-427: LGTM! Convenient API surface for common cases.The convenience functions (
load_settings(),get_config_path(), etc.) provide a clean, simple API for the most common use cases while still allowing advanced users to instantiateConfiguratordirectly with custom paths.
- Updated line-length from 100 to 88 chars (Black default) - Added ARG rule to catch unused function arguments - Fixed unused did_config_path parameter across codebase - Removed duplicate MAX_PERCENTAGE constant - Fixed dynamic constant reference in models.py - Updated did dependency reference in docs - Added editorconfig-checker for style enforcement - Added mdformat for markdown formatting - Created fix_whitespace.py script for automated cleanup - Fixed trailing whitespace in 10 files - Added LICENSE-specific editorconfig rules - All tests passing (123 tests, 87.34% coverage)
- Wrapped long docstrings to 88 chars - Fixed long field descriptions in Pydantic models - Reformatted test fixtures and mock data - Fixed indentation in docstrings - Added final newline to bin/iptax - All 123 tests passing, 87.34% coverage
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
README.md (1)
3-8: Fix minor typos and brand capitalization in README.A few small text issues worth cleaning up:
- Capitalize platform names:
GitHub/GitLab.- Fix spelling:
integration,repositories,hours.You could apply something like:
-This tool helps to calculate and report income from FOSS work in Poland, taking into -account the 50% tax deduction for copyright income. +This tool helps calculate and report income from FOSS work in Poland, taking into +account the 50% tax deduction for copyright income. -It uses psss/did project to get the merged changes from configured github/gitlab -instances. It has an intergration with the Workday to get the number of hours worked. +It uses the psss/did project to get merged changes from configured GitHub/GitLab +instances. It has an integration with Workday to get the number of hours worked. … -- Markdown report of reporitories and project changes +-- Markdown report of repositories and project changes -– Report PDF with work card reference, number of creative work hous, and co-authors +– Report PDF with work card reference, number of creative work hours, and co-authorsAlso applies to: 29-32
scripts/fix_whitespace.py (1)
6-31: Tighten exception handling infix_fileto avoid over-broad catches.The whitespace logic looks good, but the broad catch is heavier than needed:
except Exception as e: print(f"Error processing {filepath}: {e}", file=sys.stderr) return FalseSince the risky operations here are file I/O and decoding, you can narrow this and stay in line with the stricter lint/security rules you enabled:
- except Exception as e: - print(f"Error processing {filepath}: {e}", file=sys.stderr) - return False + except (OSError, UnicodeError) as exc: + print(f"Error processing {filepath}: {exc}", file=sys.stderr) + return FalseOptionally, you could also specify an explicit encoding for deterministic behavior:
content = filepath.read_text(encoding="utf-8") … filepath.write_text(content, encoding="utf-8")Also applies to: 32-57
docs/requirements.md (2)
207-222: Add language identifier to workflow code block.The workflow pseudo-code block is missing a language identifier. Add
textfor proper rendering:-``` +```text 1. Load judgment history/cache 2. Collect all changes from did
225-272: Add language identifier to prompt template code block.The AI prompt template block is missing a language identifier. Add
textfor proper rendering:-``` +```text Product: {product_name} Previous Judgment History (for context and learning):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.editorconfig(1 hunks).github/workflows/ci.yml(3 hunks)Makefile(4 hunks)README.md(2 hunks)bin/iptax(1 hunks)docs/architecture.md(1 hunks)docs/edge-cases.md(1 hunks)docs/examples.md(1 hunks)docs/implementation.md(1 hunks)docs/requirements.md(1 hunks)docs/testing.md(1 hunks)docs/workflows.md(1 hunks)pyproject.toml(5 hunks)scripts/fix_whitespace.py(1 hunks)src/iptax/__init__.py(1 hunks)src/iptax/cli.py(1 hunks)src/iptax/config/base.py(1 hunks)src/iptax/config/interactive.py(1 hunks)src/iptax/models.py(14 hunks)tests/conftest.py(2 hunks)tests/unit/test_config.py(8 hunks)tests/unit/test_models.py(8 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/iptax/cli.py
- src/iptax/init.py
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/testing.md
- docs/implementation.md
- docs/edge-cases.md
- tests/unit/test_config.py
- docs/workflows.md
🧰 Additional context used
🧬 Code graph analysis (3)
src/iptax/config/interactive.py (1)
src/iptax/models.py (9)
DidConfig(264-312)DisabledAIConfig(219-226)EmployeeInfo(36-50)GeminiProviderConfig(144-177)ProductConfig(63-73)ReportConfig(76-114)Settings(315-398)VertexAIProviderConfig(180-216)WorkdayConfig(236-261)
tests/unit/test_models.py (1)
src/iptax/models.py (4)
ProductConfig(63-73)ReportConfig(76-114)get_url(475-482)get_url(523-538)
src/iptax/config/base.py (2)
src/iptax/config/interactive.py (1)
run_interactive_wizard(28-58)src/iptax/models.py (10)
DidConfig(264-312)DisabledAIConfig(219-226)EmployeeInfo(36-50)ProductConfig(63-73)ReportConfig(76-114)Settings(315-398)WorkdayConfig(236-261)from_yaml_file(352-376)to_yaml_file(378-398)get_config_path(306-312)
🪛 checkmake (0.2.2)
Makefile
[warning] 48-48: Target body for "install" exceeds allowed length of 5 (11).
(maxbodylength)
🪛 LanguageTool
README.md
[style] ~3-~3: ‘taking into account’ might be wordy. Consider a shorter alternative.
Context: ...report income from FOSS work in Poland, taking into account the 50% tax deduction for copyright inc...
(EN_WORDINESS_PREMIUM_TAKING_INTO_ACCOUNT)
[uncategorized] ~6-~6: The official name of this software platform is spelled with a capital “H”.
Context: ... get the merged changes from configured github/gitlab instances. It has an intergratio...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
docs/requirements.md
207-207: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
225-225: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.5)
scripts/fix_whitespace.py
27-27: Consider moving this statement to an else block
(TRY300)
28-28: Do not catch blind exception: Exception
(BLE001)
src/iptax/config/base.py
128-131: Avoid specifying long messages outside the exception class
(TRY003)
136-139: Avoid specifying long messages outside the exception class
(TRY003)
141-144: Avoid specifying long messages outside the exception class
(TRY003)
146-149: Avoid specifying long messages outside the exception class
(TRY003)
165-170: Avoid specifying long messages outside the exception class
(TRY003)
173-173: Avoid specifying long messages outside the exception class
(TRY003)
176-179: Avoid specifying long messages outside the exception class
(TRY003)
189-197: Avoid specifying long messages outside the exception class
(TRY003)
236-239: Avoid specifying long messages outside the exception class
(TRY003)
280-280: Avoid specifying long messages outside the exception class
(TRY003)
src/iptax/models.py
99-101: Avoid specifying long messages outside the exception class
(TRY003)
366-366: Avoid specifying long messages outside the exception class
(TRY003)
372-374: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit Tests (Python 3.12)
🔇 Additional comments (16)
.github/workflows/ci.yml (1)
98-108: Codecov v4 setup looks fine; just confirm coverage path and secret.
files: ./coverage.xmlandif: matrix.python-version == '3.11'match the coverage step placement, and usingtoken: ${{ secrets.CODECOV_TOKEN }}is the recommended pattern for v4. Please double-check that:
make coverageactually generatescoverage.xmlat the repo root, andCODECOV_TOKENis configured in repo secrets with appropriate scope.bin/iptax (1)
12-13: LGTM – dev runner remains correct.Only formatting (final newline) changed; importing
iptax.cliand callingcli()under__main__is still appropriate for a local/dev entry point..editorconfig (1)
12-14: EditorConfig consolidation looks consistent with tooling config.Global defaults (2-space, 88 columns) plus the 4-space override for
*.py,max_line_length = offfor LICENSE, andmdformat-controlled*.mdmatch the black/ruff settings inpyproject.toml. Nothing stands out as problematic.Also applies to: 20-22, 27-29
pyproject.toml (1)
222-233: Theexclude_linespattern is correct and requires no changes.The pattern
if __name__ == .__main__.:is actually the officially recommended pattern from coverage.py documentation for excluding the typicalif __name__ == "__main__":guard. In regex, the dot (.) is a wildcard that matches any character, so.__main__.correctly matches"__main__"(with quotes). The pattern is intentional and works as designed.Makefile (1)
47-78: LGTM! Clean separation of concerns between production install and dev init.The new
installtarget for production/system installation and the renamedinittarget for development environment setup provide a clear distinction between end-user and developer workflows. The implementation correctly handles both system (/usr/local) and user (~/.local) installation paths with appropriate permissions checks.tests/unit/test_models.py (1)
93-95: LGTM! Consistent test data updates.The test data updates are consistent across the suite:
- Product name changed to "Acme Fungear" aligning with documentation examples
- GitLab host changed to generic
gitlab.example.org(appropriate for test data)- Error message assertions correctly reference the
MAX_PERCENTAGEconstant- Test signature simplification (removing unused
tmp_path) is appropriateAlso applies to: 150-161, 180-180, 508-679
docs/examples.md (1)
1-695: Excellent comprehensive examples documentation!This documentation provides thorough coverage of configuration examples, command usage, workflows, troubleshooting, and development patterns. The structure is clear with proper cross-references to other documentation files, and all code blocks include appropriate language identifiers.
src/iptax/models.py (3)
22-23: LGTM! MAX_PERCENTAGE constant improves maintainability.The
MAX_PERCENTAGEconstant centralizes the validation logic and is correctly:
- Used in the Field description (line 91)
- Used in validation logic (line 98)
- Imported in
src/iptax/config/interactive.py(line 14) avoiding duplicationThis addresses the previous concern about duplicate constants across modules.
Also applies to: 91-91, 98-101
11-11: LGTM! Improved YAML I/O and error semantics.The changes enhance the YAML handling:
- Using
path.open()with explicit encoding is more robust than implicit file handling- Changing from
ValueErrortoTypeError(line 363, 372) for invalid YAML is semantically more accurate- Maintaining proper file permissions (0o600) and directory creation
Also applies to: 368-395
537-538: LGTM! Simplified GitLab URL construction.Removing the explicit
elsebranch and adding a clarifying comment makes the code more concise while maintaining the same logic. The function correctly handles both GitHub and GitLab URL patterns.src/iptax/config/interactive.py (1)
1-359: LGTM! Well-structured interactive configuration wizard.The implementation is clean and maintainable:
- Correctly imports
MAX_PERCENTAGEfromiptax.models(line 14), avoiding duplication- Clear separation of concerns with dedicated helper functions for each configuration section
- Proper input validation using questionary's validation features
- Good defaults handling throughout
- The callback pattern for
list_providers_fnprovides clean dependency injectionsrc/iptax/config/base.py (5)
41-117: LGTM! Configurator class provides flexible configuration management.The
Configuratorclass is well-designed:
- Supports both default and custom paths (useful for testing)
- Properly respects
XDG_CONFIG_HOMEandHOMEenvironment variables- Clear separation between iptax settings and did configuration
- Static methods for default path resolution are clean and testable
118-181: LGTM! Robust loading and validation with helpful error messages.The
load()andvalidate_did_config()methods provide comprehensive validation:
- Clear error messages with actionable guidance for users
- Proper exception chaining preserves error context
- Validates file existence, readability, and YAML syntax
- Distinguishes between different error types (FileNotFoundError, YAMLError, ValueError)
199-240: LGTM! Clean integration with did SDK.The
list_did_providers()method cleanly integrates with the did SDK:
- Uses did's Config class to parse the configuration properly
- Filters out the 'general' section correctly
- Provides clear error messages when no providers are configured
- Proper exception handling with re-raising of DidConfigError
241-344: LGTM! Well-organized configuration creation flow.The
create()method and supporting helpers provide a clean configuration creation workflow:
- Validates did configuration before proceeding
- Loads existing settings for defaults when available
- Delegates to separate interactive wizard module (good separation)
- Provides minimal template fallback for non-interactive mode
- Proper error handling throughout
347-431: LGTM! Convenient API for common use cases.The convenience functions provide a simple API for the most common use cases while allowing direct use of the
Configuratorclass for more complex scenarios. This is a good balance between simplicity and flexibility.
cardil
force-pushed
the
fix/housekeeping
branch
from
37842a2 to
da26bce
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
144-145: Success notification is minimal but harmless.The echo step provides minimal functional value—actual verification is performed by
make verify. If desired, this could be enhanced with meaningful summary output (e.g., test counts, coverage percentage), but as-is it's a lightweight completion marker.scripts/fix_whitespace.py (1)
27-27: Consider moving return to an else block for clarity.As suggested by Ruff, moving
return Falseto anelseblock would make the control flow more explicit.if content != original: filepath.write_text(content, encoding='utf-8') return True - return False + else: + return False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/ci.yml(3 hunks)README.md(2 hunks)docs/architecture.md(1 hunks)docs/examples.md(1 hunks)docs/requirements.md(1 hunks)scripts/fix_whitespace.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/architecture.md
- README.md
- docs/requirements.md
🧰 Additional context used
🪛 Ruff (0.14.5)
scripts/fix_whitespace.py
27-27: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (5)
docs/examples.md (1)
1-695: Comprehensive examples documentation—all previous issues resolved. ✅This new documentation file provides excellent practical examples covering configurations, workflows, troubleshooting, development setup, and integrations. All language identifiers are in place, guard files and project structure are accurate, and the development workflow correctly uses
make init(not the oldmake install) throughout.All flagged issues from prior reviews have been addressed: YAML/bash/text language tags are present, directory examples correctly reflect the
.make/init.doneguard and the newsrc/iptax/config/package structure, and CI/development instructions align with the refactored Makefile semantics.The cross-references at the top and the quick-reference tables enhance discoverability and usability for both end-users and contributors.
.github/workflows/ci.yml (2)
75-80: Cache key reformatting looks correct.The multi-line YAML formatting for the cache key is syntactically valid and maintains the same cache-invalidation logic. The key still includes the matrix Python version and pyproject.toml hash.
98-104: Coverage generation is properly configured and will work as expected.The verification confirms that
make unitcorrectly generates./coverage.xml:
- pytest configuration (
pyproject.toml [tool.pytest.ini_options]) includes--cov=iptaxand--cov-report=xmlinaddopts, ensuring coverage reports are generated automatically.- pytest-cov is listed in dev dependencies (
pytest-cov>=4.1.0).- Coverage source is configured to
["src"]with appropriate omit patterns in[tool.coverage.run].- Makefile unit target runs
pytest tests/unit/ -v, which automatically applies the coverage settings frompyproject.toml.- Makefile refactoring (init/init.done workflow changes) does not affect coverage generation since pytest configuration is independent.
- fail_ci_if_error: false is intentional and appropriate—upload failures won't block the build, which is reasonable for non-critical CI steps.
scripts/fix_whitespace.py (2)
32-60: LGTM!The script structure is well-organized:
- Clear file pattern definitions
- Proper exclusion of cache and VCS directories
- Effective use of
rglobfor recursive file discovery- Comprehensive exception handling with stderr reporting
- Helpful summary output showing modified files
The implementation is sound and appropriate for a housekeeping script.
14-17: Line ending normalization is intentional and correct per repository standards.The script's behavior of normalizing all line endings to Unix style (
\n) is not a bug—it's the correct implementation of the repository's.editorconfigpolicy. The root[*]section explicitly specifiesend_of_line = lf, requiring Unix line endings for all files. The script correctly enforces this standard by:
- Normalizing all line endings to
\n(lines 14-17)- Ensuring files end with a newline (lines 21-22)
- Trimming trailing whitespace (line 15)
These behaviors align directly with the EditorConfig requirements (
end_of_line = lf,insert_final_newline = true,trim_trailing_whitespace = true). No changes are needed.
cardil
force-pushed
the
fix/housekeeping
branch
from
da26bce to
9fc1f64
Compare
Summary
This PR refactors the config module into a proper package, restructures documentation for better organization, and improves the build system and code quality tooling.
Code Refactoring
src/iptax/config.py→config/package (base.py,interactive.py)Configuratorclass logic from interactive wizard functionalityDocumentation Improvements
project.md(4000+ lines) into focused, well-organized documentation filesarchitecture.md,requirements.md,workflows.md,testing.md,implementation.md,edge-cases.md,examples.mdproject.mdto serve as a navigation index with cross-referencesBuild System Improvements
make install→make initto clarify it's for development environment setupmake installtarget for production installation (system/user)Code Quality Enhancements
Test plan
make initworks for dev setupmake installworks for production installationAssisted by: 🤖 Claude Sonnet 4.5
Summary by CodeRabbit
New Features
Documentation
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.