Repository navigation
Code quality improvements - #3
Merged
Merged
Conversation
Four throw sites used plain `Error`, so callers couldn't discriminate them from the library's typed error hierarchy the way they can for DateAlignmentError, NoWeightsError, etc. Add InvalidArgumentError (bad argument values) and CaptorApiError (Captor API fetch failures), reuse the existing but previously unthrown IncorrectArgumentComboError for the report generator's constituent-count check, and update tests to assert on error type in addition to message. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
types.ts had zero doc comments across its 14 exports, utils.ts left 6 of 10 functions undocumented, and roughly half of OpenTimeSeries's methods/getters in series.ts had no JSDoc. Since typedoc publishes this as the library's docs site, these gaps showed up directly in the generated reference. Document the remaining exports and add @throws tags where a typed error can be raised. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
parserOptions.project was already set (full type info was being computed on every lint run) but the config only extended tseslint.configs.recommended, so none of the type-aware rules (no-unsafe-*, no-floating-promises, etc.) were actually enabled. Switch to recommendedTypeChecked. Fixes surfaced by the switch: new Array(n).fill(x) resolves to any[], not number[], causing unsafe-return/unsafe-argument in frame.ts's and portfoliotools.ts's weight calculations - type the Array constructor calls explicitly. In scripts/compare-metrics.ts, narrow JSON.parse's result to `unknown` before use instead of implicitly typing it `any`. The remaining ~90 violations were non-null assertions and type assertions that were already redundant under the current tsconfig and are removed by --fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
npm run format:check existed as a script but nothing ran it, so a PR with unformatted code could merge and only get caught locally if someone happened to run prettier by hand. Run it alongside the existing lint/typecheck step. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
docs/ (typedoc output) was manually rebuilt and committed to master, so it could drift from source between rebuilds and every regeneration produced a large, low-signal diff. Add a workflow that builds and deploys the docs to GitHub Pages via GitHub Actions on every push to master, and stop tracking the generated output. Manual follow-up needed: this repo's GitHub Pages source (Settings > Pages) is presumably still set to "Deploy from a branch" against /docs on master. It needs to be switched to "GitHub Actions" for this workflow to actually publish the site - that's a repo setting this commit can't change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Branches sat at a 76% threshold while statements/functions/lines were gated at 95%. Investigated with istanbul's HTML report rather than guessing from the summary table: - simulation.ts (72.7% branches): entirely unused default-parameter branches (trading_days_in_year, seed, jump params) - every call site passed every argument explicitly. Added tests that omit them. - portfoliotools.ts (64.9%, the worst file): the existing "covers vol=0 branch" test used a strictly increasing price series, which doesn't actually produce zero variance - fixed it to use flat prices. Added coverage for efficientFrontier's default parameters, a singular-covariance-matrix input (duplicate assets, which the analytic solver can't resolve into a frontier), and preparePlotData's too-few-valid-returns guards. - series.ts (74.6%): ~15 risk-metric methods share a `dates.length < 2` (etc.) guard that was never exercised. One single-point-series test closes all of them at once. Net: branches 81.6% -> 85.73% overall (portfoliotools.ts alone 64.9% -> 80.7%, simulation.ts to 100%). Raised the threshold to 84% to lock in the improvement. Remaining gaps in frame.ts/report.ts/ plot.ts are mostly defensive `??` fallbacks and matrix-math edge cases (singular/degenerate inputs) that would need contrived inputs disproportionate to their value; left as documented residual rather than chased to 100%. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
The typed-error commit on this branch adds two new exported error classes (InvalidArgumentError, CaptorApiError) to the public API - an additive, backwards-compatible change, so minor per semver. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The manual "run npm run docs, commit docs/, configure Settings > Pages to deploy from a branch" instructions were superseded by the GitHub Actions docs workflow added earlier on this branch, which now builds and deploys docs automatically on push to master. npm run docs itself is still useful for a local preview, so it stays. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A round of code-quality improvements identified from a general review of the codebase, plus the version bump the API addition motivates.
Errorinstead of the library's typed error hierarchy. AddedInvalidArgumentErrorandCaptorApiError, and put the previously-unusedIncorrectArgumentComboErrorto work, so callers caninstanceof-discriminate all failure modes consistently.types.ts(0/14 exports documented),utils.ts(4/10), and roughly half ofOpenTimeSeries's methods inseries.tswere undocumented, which showed up directly in the generated typedoc site. Filled in the gaps and added@throwstags.parserOptions.projectwas already enabled (paying the type-checking cost) but the config only extendedtseslint.configs.recommended, so none of the type-aware rules (no-unsafe-*, etc.) actually ran. Switched torecommendedTypeCheckedand fixed what it surfaced (twoany[]typing gaps fromnew Array(n), an implicitanyfromJSON.parsein a script, ~90 already-redundant non-null/type assertions removed by--fix).npm run format:checkexisted as a script but nothing ran it in CI - added it alongside the existing lint/typecheck step.docs/(typedoc output) was manually rebuilt and committed tomaster, so it could drift from source and every regeneration produced a large, low-signal diff. Added a workflow that builds and deploys to GitHub Pages via GitHub Actions on every push tomaster/main, and stopped tracking the generated output. (The repo's Pages source setting has been switched to "GitHub Actions" accordingly.)simulation.ts, now 100%), avol=0test that didn't actually produce zero variance, an untested singular-covariance-matrix case, and ~15 risk-metric methods inseries.tsthat share adates.length < 2guard never exercised by a short series. Overall branches: 81.6% -> 85.73%; raised the threshold to 84%. Remaining gaps are documented in the commit as defensive/edge-case code not worth chasing to 100%.InvalidArgumentError/CaptorApiErrorexports are an additive, backwards-compatible public API change, so minor per semver.Test plan
npm run check(lint + typecheck) passesnpm run test- 306 tests passingnpm run format:checkpassesnpm run test:coveragemeets the new 84% branch threshold (85.73% actual)npx typedocbuilds cleanly with the new JSDoc🤖 Generated with Claude Code