Skip to content

fix: declare code and data properties in BaseError constructor#2294

Closed
liamhess wants to merge 4 commits into
isomorphic-git:mainfrom
liamhess:liam/fix-baseerror-types
Closed

fix: declare code and data properties in BaseError constructor#2294
liamhess wants to merge 4 commits into
isomorphic-git:mainfrom
liamhess:liam/fix-baseerror-types

Conversation

@liamhess

@liamhess liamhess commented Mar 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

While working on #2293 I stumbled upon a type error in BaseError. It's not erroring today because no test currently use the fromJSON() return type, but it would fail test.typecheck if anyone writes a test that calls fromJSON() and then accesses .code or .data on the result.

I'm fixing a bug or typo

  • if this is your first time contributing, run npm run add-contributor and follow the prompts to add yourself to the README
  • squash merge the PR with commit message "fix: [Description of fix]"

Summary by CodeRabbit

  • New Features

    • BaseError now includes error code and data fields, enabling more detailed error context and information tracking. This enhancement improves error handling across the application.

  • Tests

    • Added comprehensive test coverage for BaseError serialization and round-trip deserialization functionality.

  • Chores

    • Updated contributor acknowledgments in project documentation.

@coderabbitai

coderabbitai Bot commented Mar 11, 2026 •
edited
Loading

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Adds a contributor entry for Liam Heß to documentation config and README; extends BaseError with two instance properties (code, data) initialized in the constructor; and adds a test validating toJSON/fromJSON round-trip for those properties.

Changes

Cohort / File(s) Summary
Contributor Registry
\.all-contributorsrc, README.md		 Inserted a new contributor object for liamhess into the all-contributors configuration and added Liam Heß to the README contributor tables (content-only changes).
BaseError & tests
src/errors/BaseError.js, __tests__/test-BaseError.js		 Added code (string) and data (object) instance properties to BaseError (initialized in constructor) and updated/added tests to assert correct toJSON/fromJSON serialization and preservation of these properties.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hop with joy, new names to see,
Code and tests tucked in my tree,
Errors carry data, labels, too,
A tiny change — a brighter view! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: declaring code and data properties in the BaseError constructor, which is the primary fix across all modified files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@liamhess liamhess mentioned this pull request Mar 11, 2026
Merged
@jcubic

jcubic commented Mar 11, 2026

Copy link
Copy Markdown
Member

Please merge this with the other PR. I'm not gonna merge PR with errors so you can create another PR to fix it.

I'm not sure if you know, but you can commit after the PR was created. PR is not real only. It's just a branch reference.

@liamhess

liamhess commented Mar 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Please merge this with the other PR. I'm not gonna merge PR with errors so you can create another PR to fix it.

I'm not sure if you know, but you can commit after the PR was created. PR is not real only. It's just a branch reference.

Sorry @jcubic, I think I didn't explain clearly what I did: The BaseError type fix and the discoverGitdir fix are two separate issues. The BaseError type issue only was revealed as a side effect of the discoverGitdir fix PR and the two are really unrelated.
Now I thought it might be nice to create two separate stacked PRs for the two separate issues.
What I did is I based the #2293 on top of #2294.
main < #2294 < #2293
The merge order would be to first squash merge #2294 and then I would rebase #2293 on the latest main branch and then #2293 would have no type errors anymore and we could merge that one. This way main will always stay green and the PRs ( / squashed commits on main) are more focused and smaller in scope.

However if you don't think this is nicer then I will gladly put both fixes in one PR. Is probably less work for both of us. It's your project and I'm just happy if I can land the fix, let me know what you think :)

@jcubic

jcubic commented Mar 12, 2026

Copy link
Copy Markdown
Member

Maybe I'm missing something, but both of your PRs have the same BaseError changes that are required for the tests to pass after your changes.

@liamhess

liamhess commented Mar 12, 2026

Copy link
Copy Markdown
Contributor Author

Maybe I'm missing something, but both of your PRs have the same BaseError changes that are required for the tests to pass after your changes.

Yeah, that's confusing sorry. Usually GitHub allows you to choose the 'base branch' (right below the title) and then it only shows the commits and file diffs against that base branch. But I couldn't choose a base branch that is in a fork in #2293, maybe that is a limitation of GitHub, idk?
So that's why it showed the commits of #2294 in #2293 as well.
But anyway I read your comment #2293 (comment) and am not importing from src anymore, so the two PRs are completely independent from each other now and #2293 doesn't depend on this one anymore so no more stacking needed.
Is this PR still something that you would want? Otherwise I'll just close it.

@liamhess liamhess closed this Apr 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@liamhess @jcubic