Skip to content

Conversation

bobzhang
Copy link
Contributor

@bobzhang bobzhang commented Sep 4, 2025

cc @Yu-zh

Copy link

Inconsistent deprecation message formatting

Category
Maintainability
Code Snippet
Line 313: #deprecated("ArrayView will be immutable, use Array if you need mutation")
Line 135: #deprecated("ArrayView will be immutable, use array if you need mutation")
Recommendation
Standardize the deprecation message by using consistent capitalization: either "Array" or "array" throughout all deprecation messages
Reasoning
Inconsistent capitalization in deprecation messages can confuse users about the correct type name to use as a replacement

Incomplete deprecation scope - missing mapi_inplace

Category
Maintainability
Code Snippet
view.mbt shows map_inplace is deprecated but mapi_inplace at line 330+ is not marked as deprecated, yet it's also a mutation operation
Recommendation
Add #deprecated annotation to mapi_inplace function as well since it also mutates the ArrayView in place
Reasoning
For API consistency, all mutation operations on ArrayView should be deprecated together to provide a clear migration path

Missing swap function deprecation

Category
Correctness
Code Snippet
builtin/arrayview_test.mbt removes op_set test but keeps swap test, and swap function in builtin/arrayview.mbt is not deprecated
Recommendation
Add #deprecated annotation to the swap function since it also mutates the ArrayView
Reasoning
The swap function modifies the ArrayView contents and should be deprecated consistently with other mutation operations like op_set and map_inplace

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 1017

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.005%) to 89.612%

Totals Coverage Status
Change from base Build 1016: -0.005%
Covered Lines: 8756
Relevant Lines: 9771

💛 - Coveralls

/// ```
#deprecated("ArrayView will be immutable, use Array if you need mutation")
pub fn[T] View::map_inplace(self : View[T], f : (T) -> T raise?) -> Unit raise? {
for i, v in self {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @Guest0x0 this is another case where you want deprecation not working in the same package, we heave a function a deprecated, and a function b deprecated (b called a), but we can not simply remove b during migration period (similar to recursive ones). The workaround is the same, rename a to a private function, mark the public interface deprecated

@bobzhang bobzhang enabled auto-merge (rebase) September 4, 2025 00:45
@bobzhang bobzhang merged commit 7d4f87b into main Sep 4, 2025
14 checks passed
@bobzhang bobzhang deleted the hongbo/immutable_array_view branch September 4, 2025 00:45
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