Skip to content
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.

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

[dsch] adding the type-hidden properties error to Ok and value to Err… #6

Merged
merged 2 commits into from
Sep 26, 2024

Conversation

DScheglov
Copy link
Owner

@DScheglov DScheglov commented Sep 26, 2024

… of type never

Summary by CodeRabbit

  • New Features

    • Introduced new getter methods in ErrImpl and OkImpl classes to handle error states more explicitly.
    • Added a new dependency, terser, for improved minification.
  • Bug Fixes

    • Enhanced error handling for accessing value and error properties in Ok and Err results.
  • Tests

    • Added new test suites to validate the behavior of value and error properties for both Ok and Err results.
  • Chores

    • Reordered dependencies in package.json for better organization.

Copy link

coderabbitai bot commented Sep 26, 2024

Walkthrough

The changes involve modifications to type assertions in result variables, the introduction of new getter methods in the ErrImpl and OkImpl classes, updates to the package.json for dependency management, and enhancements to test coverage for the Result class. Additionally, the tsup.config.js file was updated to allow for more flexible configuration options, including the use of a specific minification tool.

Changes

File Change Summary
examples/methods.ts Modified type assertions for okResult and errResult using the as keyword after initialization.
package.json Reordered devDependencies and added a new dependency, terser, with version ^5.33.0.
src/Err.ts Added a new getter method value in the ErrImpl class that throws a TypeError when accessed.
src/Ok.ts Introduced a new getter method error in the OkImpl class that throws a TypeError when accessed.
src/result.test.ts Added two new test suites for Ok.value and Err.error, validating the behavior of the value and error properties.
tsup.config.js Updated method signature to include an options parameter in the entries mapping function and changed minify property to use 'terser'.

Possibly related PRs


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@coveralls
Copy link

coveralls commented Sep 26, 2024

Pull Request Test Coverage Report for Build 11054870290

Details

  • 12 of 12 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 98.285%

Totals Coverage Status
Change from base Build 10999153378: 0.01%
Covered Lines: 1353
Relevant Lines: 1371

💛 - Coveralls

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range comments (2)
src/Ok.ts (1)

Line range hint 1-93: Consider standardizing error messages for consistency

The new error getter introduces a specific error message: "Result is not an Err". For consistency, you might want to review and standardize error messages across the class. For instance, the unwrapErr method on line 52 uses the same error message.

Consider creating a constant for this error message to ensure consistency and ease of maintenance:

private static readonly NOT_ERR_MESSAGE = 'Result is not an Err';

// Then use it in both places:
get error(): never {
  throw new TypeError(OkImpl.NOT_ERR_MESSAGE, { cause: this });
}

unwrapErr(): never {
  throw new TypeError(OkImpl.NOT_ERR_MESSAGE, { cause: this });
}

This change would improve maintainability and ensure consistent error messaging throughout the class.

examples/methods.ts (1)

Inconsistent Type Assertions Detected in examples/methods.ts

Several instances of ok() and err() in examples/methods.ts lack type assertions related to Result. To ensure consistency and enhance type safety, please apply appropriate type assertions or generic type parameters to all ok() and err() usages.

  • Example without type assertion:
    const okResult = ok(42);
    const errResult = err('ERR_NOT_FOUND' as const);
  • Example with type assertion:
    const okResult = ok(42) as Result<number, 'NOT_FOUND'>;
    const errResult = err('ERR_NOT_FOUND') as Result<string, 'ERR_NOT_FOUND'>;
🔗 Analysis chain

Line range hint 1-165: Verify consistency of type assertions across examples

While the changes are isolated to lines 122-123, it's important to ensure consistency across all examples in this file. Please review the other instances of ok() and err() usage to determine if similar type assertions or generic type parameters should be applied for consistency and improved type safety.


Line range hint 1-165: Verify consistency of type assertions across examples

The previous script encountered an error recognizing the typescript file type. Please use the following updated script to search for ok() and err() usage, as well as type assertions related to Result within examples/methods.ts:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for other instances of ok() and err() that might benefit from similar changes

# Test: Search for ok() and err() usage
echo "Instances of ok() and err():"
rg --type typescript '(ok|err)\(' examples/methods.ts

# Test: Search for type assertions related to Result
echo "\nType assertions related to Result:"
rg --type typescript 'as Result<' examples/methods.ts

Length of output: 331


Script:

#!/bin/bash
# Description: Check for other instances of ok() and err() that might benefit from similar changes

# Test: Search for ok() and err() usage
echo "Instances of ok() and err():"
rg '(ok|err)\(' examples/methods.ts

# Test: Search for type assertions related to Result
echo "\nType assertions related to Result:"
rg 'as Result<' examples/methods.ts

Length of output: 1786

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between 6f24340 and 892d608.

📒 Files selected for processing (6)
  • examples/methods.ts (1 hunks)
  • package.json (1 hunks)
  • src/Err.ts (1 hunks)
  • src/Ok.ts (1 hunks)
  • src/result.test.ts (1 hunks)
  • tsup.config.js (2 hunks)
🔇 Additional comments (11)
tsup.config.js (2)

17-17: Excellent enhancement to configuration flexibility!

The addition of the options parameter to the mapping function and spreading it into the configuration object is a great improvement. This change allows for more granular control over each entry's configuration, making the setup more versatile and easier to customize on a per-entry basis.

Also applies to: 31-31


27-27: Enabling minification with Terser: Good for production, verify impact.

Changing minify to 'terser' is a good optimization for production builds, as it will result in smaller output files. However, it's important to ensure this change doesn't introduce any unexpected behavior.

Please verify the following:

  1. The minified output works as expected in all target environments.
  2. This change aligns with the project's performance goals and debugging requirements.

You can use the following script to compare the output sizes and check for any potential issues:

src/Err.ts (2)

Line range hint 1-119: Summary of changes to src/Err.ts

The addition of the value getter to the ErrImpl class is a positive change that enhances type safety and consistency in the error handling of the library. It provides a clear and type-safe way to prevent access to a non-existent value on Err instances.

Key points:

  1. The new getter aligns well with TypeScript best practices.
  2. It maintains consistency with other error-throwing methods in the class.
  3. The change doesn't affect existing methods or their implementations.

To ensure a smooth integration of this change:

  1. Consider improving the error message for clarity.
  2. Update the project documentation to reflect this new behavior.
  3. Verify that this change doesn't introduce unexpected errors in other parts of the codebase.

Overall, this is a well-implemented improvement to the ErrImpl class.


16-19: Verify the impact of the new value getter on the codebase.

The addition of the value getter to ErrImpl is a good improvement for type safety. However, we should ensure that this change doesn't introduce unexpected errors in other parts of the codebase where value might be accessed on a Result type without first checking if it's Ok.

Run the following script to check for potential issues:

If the script finds any matches, please review those occurrences to ensure they properly check for isOk before accessing value.

src/Ok.ts (1)

17-19: Excellent addition of the error getter!

The new error getter method is a valuable addition to the OkImpl class. It enhances type safety and improves the developer experience by providing immediate, clear feedback when attempting to access the error property on an Ok result.

Key benefits:

  1. Consistent with TypeScript's never type, indicating that this method should never return normally.
  2. Provides a descriptive error message, aiding in debugging and preventing misuse.
  3. Likely creates symmetry with a similar value getter on the Err type, resulting in a more consistent API.

This change aligns well with TypeScript's type system and error handling best practices.

package.json (3)

34-45: Summary of package.json changes

The changes in the package.json file are primarily focused on dependency management:

  1. Reordering of some dependencies for better organization.
  2. Addition of terser for improved minification capabilities.
  3. Updates to typescript and typescript-eslint versions.

These changes align with the PR objectives by potentially improving build output and keeping the project up-to-date with the latest TypeScript features and best practices. However, it's important to ensure that the TypeScript updates don't introduce any breaking changes to the existing codebase.


40-40: New dependency added: terser

The addition of terser as a dev dependency is likely related to minification improvements mentioned in the AI summary. This is a positive change that can help reduce the size of the built package.

Let's verify if terser is being used in the build process:

✅ Verification successful

Verified: terser is correctly integrated into the build process.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if terser is used in any configuration files

# Test: Search for terser usage in configuration files
rg --type json --type js 'terser' -A 5

Length of output: 479


44-45: Updated TypeScript-related dependencies

The typescript and typescript-eslint dependencies have been updated to newer versions. This is generally good for keeping up with the latest features and bug fixes. However, it's important to ensure that these updates don't introduce any breaking changes.

Let's verify if there are any TypeScript-related issues after the update:

src/result.test.ts (3)

124-134: LGTM! Well-structured test suite for Ok.value.

The test suite for Ok.value is well-implemented:

  1. It correctly tests the happy path by verifying the value property of an Ok result.
  2. It also checks the error case by ensuring that accessing the value property of an Err result throws the expected TypeError.

These tests provide good coverage for the value property behavior.


136-146: LGTM! Well-structured test suite for Err.error.

The test suite for Err.error is well-implemented:

  1. It correctly tests the happy path by verifying the error property of an Err result.
  2. It also checks the error case by ensuring that accessing the error property of an Ok result throws the expected TypeError.

These tests provide good coverage for the error property behavior and maintain consistency with the Ok.value test suite structure.


124-146: Great addition to the test suite!

The new test suites for Ok.value and Err.error are well-structured and provide comprehensive coverage for these properties. They test both the happy path and error cases, ensuring robust behavior of the Result class. These additions improve the overall quality and reliability of the codebase.

src/Err.ts Outdated
Comment on lines 16 to 19
get value(): never {
throw new TypeError('Result is not an Ok', { cause: this });
}

Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Approve the addition of the value getter with suggestions for improvement.

The new value getter is a good addition to the ErrImpl class. It enhances type safety and provides a consistent way to handle attempts to access the value property on an Err instance. The implementation is correct and aligns well with TypeScript best practices.

However, consider the following suggestions for improvement:

  1. The error message could be more specific to help developers understand why they can't access value on an Err. For example:

    throw new TypeError('Cannot access `value` on an Err instance. Use `error` property instead.', { cause: this });
  2. Ensure that this change is documented in the project's documentation or changelog to inform users of this new behavior.

examples/methods.ts Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between 892d608 and b2349eb.

📒 Files selected for processing (3)
  • src/Err.ts (1 hunks)
  • src/Ok.ts (1 hunks)
  • src/result.test.ts (1 hunks)
🔇 Additional comments (4)
src/Err.ts (1)

16-20: 🧹 Nitpick (assertive)

Approve the addition of the value getter with suggestions for improvement.

The new value getter is a valuable addition to the ErrImpl class. It enhances type safety by explicitly throwing an error when attempting to access value on an Err instance. The implementation is correct and aligns well with TypeScript best practices.

However, consider the following suggestions for further improvement:

  1. Make the error message more informative to help developers understand why they can't access value on an Err. For example:

    throw new TypeError('Cannot access `value` on an Err instance. Use `error` property instead.', { cause: this });
  2. Ensure that this change is documented in the project's documentation or changelog to inform users of this new behavior.

src/result.test.ts (3)

124-136: LGTM! Well-structured tests for Ok.value.

The test suite for Ok.value is well-designed, covering both the successful case and the error case. It ensures that:

  1. The value property of an Ok result returns the correct value.
  2. Accessing the value property of an Err result throws a TypeError with the appropriate message and cause.

These tests effectively validate the behavior of the value property for both Ok and Err instances.


138-150: LGTM! Well-structured tests for Err.error.

The test suite for Err.error is well-designed and symmetric to the Ok.value suite, which is excellent for consistency. It covers:

  1. The error property of an Err result returns the correct error message.
  2. Accessing the error property of an Ok result throws a TypeError with the appropriate message and cause.

These tests effectively validate the behavior of the error property for both Err and Ok instances.


124-150: Excellent addition to the test suite!

The new test suites for Ok.value and Err.error are well-implemented and provide valuable coverage for the Result class. They effectively test both the successful and error cases for accessing the value and error properties on Ok and Err instances. The symmetry between the two test suites enhances readability and maintainability. These additions will help ensure the robustness of the Result class implementation.

src/Ok.ts Show resolved Hide resolved
@DScheglov DScheglov merged commit abebd54 into main Sep 26, 2024
7 checks passed
@DScheglov DScheglov deleted the feature/ok-error-err-value branch September 26, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants