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

make Failure pub(readonly) #1517

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tonyfettes
Copy link
Contributor

This PR makes Failure has visibility pub(readonly).

Copy link

peter-jerry-ye-code-review bot commented Jan 21, 2025

‼️ This code review is generated by a bot. Please verify the content before trusting it.

From the provided git diff, here are potential issues I've noticed:

  1. Inconsistency in visibility modifiers: The Failure type was changed from pub(all) to pub, which might affect code that was relying on the previous visibility scope. This could cause compilation errors in code that was using the pub(all) access level.

  2. Direct constructor usage vs. new method: In the test case, the code is updated to use Failure::new() but there might be other places in the codebase still using the direct constructor Failure(). These instances should be updated to use the new constructor method for consistency.

  3. Missing documentation: The newly added Failure::new() constructor method lacks documentation comments (///), which is inconsistent with the documentation style visible in other parts of the code. This could make the API harder to understand for other developers.

These are potential issues that might need attention, though their severity depends on the specific context and requirements of your project.

@tonyfettes tonyfettes requested a review from bobzhang January 21, 2025 03:53
@coveralls
Copy link
Collaborator

coveralls commented Jan 21, 2025

Pull Request Test Coverage Report for Build 4924

Details

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 82.926%

Changes Missing Coverage Covered Lines Changed/Added Lines %
builtin/failure.mbt 0 1 0.0%
Totals Coverage Status
Change from base Build 4921: -0.01%
Covered Lines: 5051
Relevant Lines: 6091

💛 - Coveralls

@tonyfettes tonyfettes force-pushed the make-failure-pub-readonly branch 2 times, most recently from ac7043c to a08c766 Compare January 21, 2025 04:27
@tonyfettes tonyfettes force-pushed the make-failure-pub-readonly branch from a08c766 to 9712f92 Compare January 22, 2025 02:24
@bobzhang
Copy link
Contributor

what's the motivation here?

@tonyfettes
Copy link
Contributor Author

I'm not sure on this change as well. My motivations for this change is, providing a smart constructor to error type allow us to:

  1. Change the inner implementation without breaking user code,
  2. We can deprecate it.

However here are my cons for this change:

  1. This will break user code anyway,
  2. Poor performance (but it's less important since we're through error).

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.

3 participants