Report includes the type/nature of row #17

Merged

natercio merged 11 commits from record-type into main 2025-11-24 16:29:44 +00:00

Conversation 2 Commits 11 Files Changed 13 +509 -49

natercio commented 2025-11-24 16:08:12 +00:00

Owner

Copy Link

No description provided.

natercio added 7 commits 2025-11-24 16:08:12 +00:00

add new Nature method to Record interface b4b12ad625

add Nature type ef0a4476a7

add OpenFIGI adaptar with tests 23614d51db

add Nature method to Record type 6b5552b559

add string method to the Nature type a1ea13ff2f

check isin format and enforce rate limit 8c784f3b74

report show nature c323047175

natercio commented 2025-11-24 16:09:27 +00:00

Author

Owner

Copy Link

@claude review this PR

@claude review this PR

gitea-actionsbot commented 2025-11-24 16:09:54 +00:00

Copy Link

Claude finished @natercio's task —— View job

---

PR Review: Report includes the type/nature of row

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze the new nature classification system
• Review OpenFIGI integration implementation
• Examine changes to record handling
• Check test coverage and quality
• Review code style and best practices
• Provide comprehensive feedback

---

Summary

This PR adds nature/type classification to trading records by integrating with the OpenFIGI API to determine whether securities are stocks (G01) or ETFs/funds (G20). This enhances the report output to properly categorize transactions for Portuguese tax reporting.

✅ Strengths

Architecture & Design

• Clean separation of concerns: Nature logic isolated in dedicated files
• Deferred evaluation: natureGetter with sync.OnceValue prevents unnecessary API calls
• Rate limiting: Proper implementation following OpenFIGI's 25 requests/minute limit
• Type safety: Strong typing with Nature enum and constants

Code Quality

• Comprehensive testing: Good test coverage including edge cases and error scenarios
• Error handling: Graceful degradation to NatureUnknown when API calls fail
• ISIN validation: Validates format and country code before making API requests

⚠️ Issues & Recommendations

Critical Issues

1. Bug in report.go:120-121 - Taxes calculation error:

   Fees:  buy.Fees().Add(rec.Fees()),
   Taxes: buy.Taxes().Add(rec.Fees()), // ← Should be rec.Taxes()
   

   This incorrectly adds sell-side fees instead of taxes.

High Priority Issues

2. Test naming inconsistency in nature_test.go:9:

   • Function name: TestNature_StringUnknow (typo: "Unknow")
   • Test names: All say "return unknown" instead of descriptive names

3. Missing HTTP timeout in open_figi.go:41:

   • API requests have no timeout, could hang indefinitely
   • Add context timeout or use client with timeout

4. ISIN validation edge case in open_figi.go:29:

   • Only validates country prefix, not full ISIN checksum
   • Consider using a proper ISIN validation library

Medium Priority Issues

5. Test typo in open_figi_test.go:36: "bas status code" → "bad status code"

6. Comments typos in record.go:

   • Line 173: "sligh parameter changes" → "slight parameter changes"
   • Line 180: "parseOptinalDecimal" → "parseOptionalDecimal" (function name too)

7. Hardcoded API URL in open_figi.go:41:

   • Consider making the base URL configurable for testing/mocking

Low Priority Issues

8. Magic numbers: The rate limiter values could be constants
9. Error messages: Could include more context (e.g., which field failed parsing)

🔍 Security & Performance

Security: ✅ Good

• No API keys or sensitive data exposure
• Proper error handling without information leakage
• Rate limiting prevents abuse

Performance: ✅ Good

• Lazy evaluation with sync.OnceValue
• Rate limiting prevents API overload
• Efficient decimal arithmetic usage

📋 Testing

Coverage: ✅ Excellent

• Comprehensive test suites for all new functionality
• Good edge case coverage (malformed data, API failures)
• Proper mock usage for external dependencies

Quality: ⚠️ Some issues

• Test naming could be more descriptive
• Function name typo needs fixing

🏗️ Integration

The changes integrate well with the existing codebase:

• Extends the Record interface cleanly
• Maintains backward compatibility
• Follows established patterns

Recommendations

1. Fix the critical tax calculation bug immediately
2. Fix test naming and typos
3. Add HTTP timeouts to API requests
4. Consider more robust ISIN validation
5. Make API URL configurable

The implementation is solid overall but needs the critical bug fix before merging.

---

**Claude finished @natercio's task** —— [View job](http://gitea:3000/applications/any2anexoj/actions/runs/73) --- ### PR Review: Report includes the type/nature of row #### Todo List: - [x] Read repository guidelines (CLAUDE.md) - [x] Analyze the new nature classification system - [x] Review OpenFIGI integration implementation - [x] Examine changes to record handling - [x] Check test coverage and quality - [x] Review code style and best practices - [x] Provide comprehensive feedback --- ### Summary This PR adds nature/type classification to trading records by integrating with the OpenFIGI API to determine whether securities are stocks (G01) or ETFs/funds (G20). This enhances the report output to properly categorize transactions for Portuguese tax reporting. ### ✅ Strengths #### Architecture & Design - **Clean separation of concerns**: Nature logic isolated in dedicated files - **Deferred evaluation**: `natureGetter` with `sync.OnceValue` prevents unnecessary API calls - **Rate limiting**: Proper implementation following OpenFIGI's 25 requests/minute limit - **Type safety**: Strong typing with `Nature` enum and constants #### Code Quality - **Comprehensive testing**: Good test coverage including edge cases and error scenarios - **Error handling**: Graceful degradation to `NatureUnknown` when API calls fail - **ISIN validation**: Validates format and country code before making API requests ### ⚠️ Issues & Recommendations #### Critical Issues 1. **Bug in report.go:120-121** - Taxes calculation error: ```go Fees: buy.Fees().Add(rec.Fees()), Taxes: buy.Taxes().Add(rec.Fees()), // ← Should be rec.Taxes() ``` This incorrectly adds sell-side fees instead of taxes. #### High Priority Issues 2. **Test naming inconsistency** in `nature_test.go:9`: - Function name: `TestNature_StringUnknow` (typo: "Unknow") - Test names: All say "return unknown" instead of descriptive names 3. **Missing HTTP timeout** in `open_figi.go:41`: - API requests have no timeout, could hang indefinitely - Add context timeout or use client with timeout 4. **ISIN validation edge case** in `open_figi.go:29`: - Only validates country prefix, not full ISIN checksum - Consider using a proper ISIN validation library #### Medium Priority Issues 5. **Test typo** in `open_figi_test.go:36`: "bas status code" → "bad status code" 6. **Comments typos** in `record.go`: - Line 173: "sligh parameter changes" → "slight parameter changes" - Line 180: "parseOptinalDecimal" → "parseOptionalDecimal" (function name too) 7. **Hardcoded API URL** in `open_figi.go:41`: - Consider making the base URL configurable for testing/mocking #### Low Priority Issues 8. **Magic numbers**: The rate limiter values could be constants 9. **Error messages**: Could include more context (e.g., which field failed parsing) ### 🔍 Security & Performance **Security**: ✅ Good - No API keys or sensitive data exposure - Proper error handling without information leakage - Rate limiting prevents abuse **Performance**: ✅ Good - Lazy evaluation with `sync.OnceValue` - Rate limiting prevents API overload - Efficient decimal arithmetic usage ### 📋 Testing **Coverage**: ✅ Excellent - Comprehensive test suites for all new functionality - Good edge case coverage (malformed data, API failures) - Proper mock usage for external dependencies **Quality**: ⚠️ Some issues - Test naming could be more descriptive - Function name typo needs fixing ### 🏗️ Integration The changes integrate well with the existing codebase: - Extends the `Record` interface cleanly - Maintains backward compatibility - Follows established patterns ### Recommendations 1. **Fix the critical tax calculation bug immediately** 2. Fix test naming and typos 3. Add HTTP timeouts to API requests 4. Consider more robust ISIN validation 5. Make API URL configurable The implementation is solid overall but needs the critical bug fix before merging. ---

natercio added 1 commit 2025-11-24 16:15:24 +00:00

fix isin in tests 93f1dab3d2

natercio added 2 commits 2025-11-24 16:23:59 +00:00

fix critical tax calculation bd101ce46a

fix typos and copy paste test names 70466b7886

natercio added 1 commit 2025-11-24 16:27:46 +00:00

test expects Nature method call 5f13ebaf6a

natercio merged commit 0d3e3df9e7 into main 2025-11-24 16:29:44 +00:00

natercio deleted branch record-type 2025-11-24 16:29:44 +00:00

natercio referenced this issue from a commit 2025-11-24 16:29:46 +00:00

Merge pull request 'Report includes the type/nature of row' (#17) from record-type into main

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

No items

No Label

Milestone

No items

No Milestone

Assignees

Clear assignees

natercio

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: applications/any2anexoj#17