feat: Add exporters and enrichers #5
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds export and data enrichment capabilities to the broker-parser library, enabling users to convert parsed transactions to Yahoo Finance CSV format and enrich transaction data with ticker symbols.
Changes:
- Added exporter functionality with a Yahoo Finance CSV exporter implementation
- Added enricher functionality to resolve ticker symbols using custom resolver functions with built-in caching
- Updated public API to export new exporter types and enricher functions
- Updated documentation with usage examples for both features
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 37 comments.
Show a summary per file
| File | Description |
|---|---|
| src/exporters/types.ts | Defines TypeScript interfaces for exporters (ExportResult and PortfolioExporter) |
| src/exporters/yahoo.ts | Implements Yahoo Finance CSV exporter with proper formatting and CSV escaping |
| src/enricher.ts | Implements transaction enrichment with ticker resolution, caching, and error handling |
| src/index.ts | Exports new exporter types and enricher functionality to public API |
| tests/exporters/yahoo.test.ts | Provides test coverage for Yahoo Finance exporter functionality |
| tests/enricher.test.ts | Provides test coverage for enricher caching and resolution logic |
| README.md | Documents usage examples for exporting and enriching transaction data |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #5 +/- ##
============================================
- Coverage 100.00% 82.21% -17.79%
============================================
Files 4 11 +7
Lines 246 928 +682
Branches 68 188 +120
============================================
+ Hits 246 763 +517
- Misses 0 165 +165
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 37 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 36 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 37 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 38 changed files in this pull request and generated 5 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Read version from package.json at runtime | ||
| const packageJson = JSON.parse( | ||
| fs.readFileSync(path.join(__dirname, '../package.json'), 'utf8') | ||
| ); | ||
|
|
||
| const program = new Command(); | ||
|
|
||
| program | ||
| .name('broker-parser') | ||
| .description('Parse broker transaction CSVs and export to other formats') | ||
| .version(packageJson.version); |
There was a problem hiding this comment.
The package.json reading logic uses __dirname which points to the dist directory after compilation. This works correctly as the package.json is located at '../package.json' relative to dist/cli.js. However, if the dist folder structure changes or the file is run in a different context, this could fail. Consider using a more robust approach like importing the package.json directly (which is now possible with resolveJsonModule: true in tsconfig.json) or handling the error case if the file is not found.
| // Read version from package.json at runtime | |
| const packageJson = JSON.parse( | |
| fs.readFileSync(path.join(__dirname, '../package.json'), 'utf8') | |
| ); | |
| const program = new Command(); | |
| program | |
| .name('broker-parser') | |
| .description('Parse broker transaction CSVs and export to other formats') | |
| .version(packageJson.version); | |
| // Read version from package.json at runtime in a robust way | |
| let packageVersion = '0.0.0'; | |
| try { | |
| const packageJsonPath = path.resolve(__dirname, '../package.json'); | |
| const packageJsonContent = fs.readFileSync(packageJsonPath, 'utf8'); | |
| const parsedPackageJson = JSON.parse(packageJsonContent); | |
| if (parsedPackageJson && typeof parsedPackageJson.version === 'string') { | |
| packageVersion = parsedPackageJson.version; | |
| } | |
| } catch { | |
| // Fallback to default version if package.json cannot be read or parsed | |
| } | |
| const program = new Command(); | |
| program | |
| .name('broker-parser') | |
| .description('Parse broker transaction CSVs and export to other formats') | |
| .version(packageVersion); |
|
|
||
| // 1. File Resolver (Highest priority if provided) | ||
| if (options.tickerFile) { | ||
| resolvers.push(new FileTickerResolver(path.resolve(options.tickerFile))); | ||
| } | ||
|
|
||
| // 2. Yahoo ISIN Resolver (High accuracy) | ||
| if (options.yahoo !== false || options.yahooIsin) { | ||
| resolvers.push(new YahooISINResolver()); | ||
| } | ||
|
|
||
| // 3. Yahoo Name Resolver (Fallback) | ||
| if (options.yahoo !== false || options.yahooName) { | ||
| resolvers.push(new YahooNameResolver()); |
There was a problem hiding this comment.
The resolver logic has a potential issue: when both --yahoo-isin and --yahoo-name are specified along with --yahoo (which defaults to true), the resolvers will be added twice. For example, if --yahoo (true by default) and --yahoo-isin are both present, YahooISINResolver gets added at line 101, creating duplicate resolvers in the stack. Consider making these options mutually exclusive or checking if a resolver is already in the list before adding.
| // 1. File Resolver (Highest priority if provided) | |
| if (options.tickerFile) { | |
| resolvers.push(new FileTickerResolver(path.resolve(options.tickerFile))); | |
| } | |
| // 2. Yahoo ISIN Resolver (High accuracy) | |
| if (options.yahoo !== false || options.yahooIsin) { | |
| resolvers.push(new YahooISINResolver()); | |
| } | |
| // 3. Yahoo Name Resolver (Fallback) | |
| if (options.yahoo !== false || options.yahooName) { | |
| resolvers.push(new YahooNameResolver()); | |
| const addedResolverTypes = new Set<string>(); | |
| // 1. File Resolver (Highest priority if provided) | |
| if (options.tickerFile && !addedResolverTypes.has('FileTickerResolver')) { | |
| resolvers.push(new FileTickerResolver(path.resolve(options.tickerFile))); | |
| addedResolverTypes.add('FileTickerResolver'); | |
| } | |
| // 2. Yahoo ISIN Resolver (High accuracy) | |
| if ((options.yahoo !== false || options.yahooIsin) && !addedResolverTypes.has('YahooISINResolver')) { | |
| resolvers.push(new YahooISINResolver()); | |
| addedResolverTypes.add('YahooISINResolver'); | |
| } | |
| // 3. Yahoo Name Resolver (Fallback) | |
| if ((options.yahoo !== false || options.yahooName) && !addedResolverTypes.has('YahooNameResolver')) { | |
| resolvers.push(new YahooNameResolver()); | |
| addedResolverTypes.add('YahooNameResolver'); |
|
|
||
| const classMatch = | ||
| name.match(/\s+Class\s+([A-Z])$/i) || name.match(/\s+([A-Z])$/); | ||
| if (classMatch) { |
There was a problem hiding this comment.
The regex pattern uses 'classMatch.index' which can be undefined in TypeScript if the match is null. Although there's a check for 'if (classMatch)', TypeScript doesn't guarantee that 'index' is defined on the match object. This should use non-null assertion (classMatch.index!) or add an additional check to ensure index is not undefined before using it in substring.
| if (classMatch) { | |
| if (classMatch && classMatch.index !== undefined) { |
| 2023-03-10,Pension,Byte,DNB Fund Technology A SEK Acc,0,1649,,,,,LU2553959045,, | ||
| 2023-03-10,Pension,Byte,DNB TECHNOLOGY,-0,1649,,,,,LU0302296495,, |
There was a problem hiding this comment.
The CSV data appears to have formatting issues with the "Antal" (quantity) field. Lines 5 and 6 show values like "0,1649" and "-0,1649" but they are split with a comma separator which might be interpreted as a field delimiter rather than a decimal separator. The values should be properly quoted or the decimal format should be consistent with the rest of the CSV (using comma as decimal separator should work if properly handled by the parser).
| // Ensure cache is saved on process exit | ||
| process.on('beforeExit', () => { | ||
| this.flush(); | ||
| }); |
There was a problem hiding this comment.
The cache registers a 'beforeExit' event listener in the constructor, but if multiple instances of LocalFileTickerCache are created (e.g., in tests or multiple CLI invocations in the same process), this could lead to memory leaks from accumulated event listeners. Consider removing the listener in a destructor/cleanup method or using 'once' instead of 'on'.
No description provided.