Skip to content

fix(parser): raise InvalidTaxCodeError when birthplace not found #72

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.

Sign up for GitHub

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

Open
matteoredz wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(parser): raise InvalidTaxCodeError when birthplace not found #72

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ ItaxCode.decode(tax_code) #=> Hash:
# code: String, # the input code, upcased
# gender: "M" | "F",
# birthdate: String, # "YYYY-MM-DD"
# birthplace: { # nil if not found in either CSV
# birthplace: { # raises InvalidTaxCodeError if not found in either CSV

@coderabbitai coderabbitai Bot May 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

InvalidTaxCodeError documentation is now incomplete across sections.

The decode section was updated, but the Error Hierarchy still describes InvalidTaxCodeError only as a length error; it should also mention unknown birthplace lookup failure for consistency.

Based on learnings: "Whenever you modify code, check whether the change affects any section of this file ... and update the relevant section in the same commit."

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@AGENTS.md` at line 81, Update the Error Hierarchy documentation to reflect
the expanded behavior of InvalidTaxCodeError: mention both the existing
length/format validation failure and the new unknown birthplace lookup failure
(e.g., when birthplace CSV lookup fails for "birthplace"). Locate the Error
Hierarchy section and add a sentence alongside the InvalidTaxCodeError entry
indicating it is raised for invalid length/format AND when birthplace lookup is
not found, matching the decode section wording for consistency.

# code: String, # e.g. "F205"
# province: String, # e.g. "MI"
# name: String, # e.g. "MILANO"
Expand Down
8 changes: 5 additions & 3 deletions lib/itax_code/parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,12 @@ def birthplace(src = utils.cities, stop: false)
place = src.find { |item| item["code"] == birthplace_code && in_dates?(item) }

if place.nil?
birthplace(utils.countries, stop: true) unless stop
else
place.to_h.transform_keys(&:to_sym)
return birthplace(utils.countries, stop: true) unless stop

raise InvalidTaxCodeError
end

place.to_h.transform_keys(&:to_sym)
end

def birthplace_code
Expand Down
5 changes: 2 additions & 3 deletions test/itax_code/parser_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,8 @@ class ParserTest < Minitest::Test
end
end

# FIXME: This behaviour needs to be fixed, maybe by raising an InvalidTaxCodeError.
test "returns nil birthplace when the lookup on both cities and countries fails" do
assert_nil klass.new("BRRDRN70M41ZXXXE").decode[:birthplace]
test "raises InvalidTaxCodeError when birthplace code is not found in cities or countries" do
assert_raises(klass::InvalidTaxCodeError) { klass.new("BRRDRN70M41ZXXXE").decode }
Comment on lines +32 to +33

@coderabbitai coderabbitai Bot May 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Freeze time in this new decode test to keep it deterministic.

This assertion exercises date-dependent decode logic (Date.today path in parser), so it should be wrapped in Timecop.freeze to prevent future time-based flakes.

As per coding guidelines: "Use Timecop.freeze for any date-sensitive tests to control time".

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/itax_code/parser_test.rb` around lines 32 - 33, This test relies on
Date.today during klass.new("BRRDRN70M41ZXXXE").decode so make it deterministic
by wrapping the assertion in a Timecop.freeze block (choose a fixed Date, e.g. a
specific YYYY,MM,DD) around the assert_raises call; locate the test case that
calls klass.new(...).decode and change it to use Timecop.freeze (block form) so
the date-dependent decode path is stable.

end

test "raises NoTaxCodeError with an empty tax code" do
Expand Down
Loading