Skip to content

add proper attribute to view types #2160

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

Merged
merged 1 commit into from
May 23, 2025
Merged

Conversation

Yu-zh
Copy link
Collaborator

@Yu-zh Yu-zh commented May 23, 2025

  • Added #builtin.valtype mainly for documentation purpose
  • Deprecated @string.StringView in favor of @string.View

Copy link

Inconsistent deprecation annotations between files

Category
Maintainability
Code Snippet
file: string/view.mbt
#deprecated("use @string.View instead")

file: builtin/arrayview.mbt
#deprecated("use @array.View instead")
Recommendation
Ensure deprecation messages follow the same format across all files, preferably with information about when it will be removed and migration steps if needed
Reasoning
Consistent deprecation messages help developers understand when and how to migrate their code. Consider adding version numbers or timeline for removal.

Missing documentation for new #builtin.valtype annotation

Category
Maintainability
Code Snippet
#builtin.valtype
struct ArrayView[T] {
buf : UninitializedArray[T]
start : Int
}
Recommendation
Add documentation comments explaining the purpose and implications of the #builtin.valtype annotation
Reasoning
Since this annotation is mainly for documentation purposes (as stated in PR message), its meaning and impact should be clearly documented for other developers

Function signature update needed in other files using StringView

Category
Correctness
Code Snippet
fn check_and_consume_base(
view : @string.StringView,
base : Int
) -> (Int, @string.StringView, Bool)!StrConvError
Recommendation
Search and update all other files that might still be using @string.StringView to use @string.View instead
Reasoning
To maintain consistency and prevent future issues, all occurrences of the deprecated type should be updated to use the new type

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 6909

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.454%

Totals Coverage Status
Change from base Build 6906: 0.0%
Covered Lines: 8687
Relevant Lines: 9396

💛 - Coveralls

@bobzhang bobzhang merged commit 19021eb into moonbitlang:main May 23, 2025
12 checks passed
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