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

deprecate Deque::truncate, rename it to unsafe_truncate #1483

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

illusory0x0
Copy link
Contributor

@illusory0x0 illusory0x0 commented Jan 15, 2025

if a API might panic, I think rename it to unsafe_xxx would be better, or we can handle the panic case make it safe,

like this.

/// @alert unsafe "Panic if the array is empty."
pub fn Array::unsafe_pop[T](self : Array[T]) -> T

related issues.

Copy link

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

Here are three observations from the provided git diff output:

  1. Deprecation of truncate Function:

    • The truncate function has been deprecated in favor of unsafe_truncate. This change is marked with a @alert deprecated annotation. However, the deprecation message could be more informative by explaining why unsafe_truncate is preferred or what risks are associated with the old truncate function. This would help developers understand the rationale behind the change and encourage them to migrate to the new function.
  2. Inconsistent Documentation:

    • The documentation for the unsafe_truncate function includes an @alert unsafe annotation warning about potential panics if len < 0. However, the original truncate function did not have such a warning, even though it presumably had the same behavior. This inconsistency in documentation could lead to confusion or unexpected behavior for developers who are not aware of the change.
  3. Test Case Naming and Updates:

    • The test cases have been updated to use unsafe_truncate instead of truncate, which is consistent with the function renaming. However, the test names have also been updated (e.g., test "deque truncate" to test "deque unsafe_truncate"). While this is correct, it might be worth considering whether the test names should reflect the deprecated function name for historical context or if they should strictly align with the new function name. This is a minor point but could be relevant for maintaining test clarity over time.

These observations highlight areas where documentation and naming could be improved to ensure clarity and consistency in the codebase.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 4771

Details

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 82.882%

Totals Coverage Status
Change from base Build 4770: 0.0%
Covered Lines: 4861
Relevant Lines: 5865

💛 - Coveralls

@peter-jerry-ye
Copy link
Collaborator

All the op_get would be unsafe. We need to discuss this later.

@Lampese
Copy link
Collaborator

Lampese commented Jan 15, 2025

All the op_get would be unsafe. We need to discuss this later.

*ex: Map

@peter-jerry-ye
Copy link
Collaborator

Map was designed exceptionally because of the 'map pattern matching', which can also be discussed whether op_get should be used or some other function should be used...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants