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

add separate packages for builtin types #1502

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

Conversation

Guest0x0
Copy link
Contributor

In #1472, the API style of builtin types and other types is different. Types defined in a regular package such as @hashmap will favor the syntax @hashmap.new() for calling things, while builtin types favor the syntax Map::new(). This is because some types in @builtin don't have their own package, such as Map/Set/Iter/StringBuilder.

This divergence of API style is bad, because sometimes it forbids abstraction over data structure. For example, if a user is using @buffer.T for internal use, but later find out that StringBuilder has better performance. If StrnigBuilder has its own package, the user can import @stringbuilder and alias it to @buffer, then everything should just work because @buffer.T and StringBuilder have similar API. But currently this is not possible because StringBuilder does not have its own package.

This PR add new packages for builtin types Map/Set/Iter/Iter2/StringBuilder. These new packages contain wrappers to the actual implementation in @builtin. This way, the API of these types would be more consistent with other types. In the future, we may also refactor @builtin and move the implementations to separated packages as well.

@Guest0x0 Guest0x0 requested a review from bobzhang January 17, 2025 09:10
Copy link

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

I'll analyze the git diff and point out potential issues I've noticed:

  1. Potential Copyright Year Issue:
    All files have a copyright year of 2025 in their headers, which appears to be a future date. This might need to be updated to the current year or the actual copyright year.

    // Copyright 2025 International Digital Economy Academy
    
  2. Type Alias Consistency:
    There's an inconsistency in how type aliases are defined across modules. For example:

    • iter/iter.mbt uses T[X]
    • iter2/iter2.mbt uses T[X, Y]
    • stringbuilder/stringbuilder.mbt uses just T
      Consider standardizing the naming convention for type aliases across modules.
  3. Function Parameter Documentation:
    In map/map.mbt, the new function has a default parameter value that's not fully specified in the interface file:

    // In map.mbti:
    fn new[K, V](capacity~ : Int = ..) -> Map[K, V]
    

    This might benefit from explicit documentation of the default value (which appears to be 8 based on the implementation).

These issues are mostly related to consistency and documentation rather than functional problems, but addressing them would improve the codebase's maintainability.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 4813

Details

  • 0 of 93 (0.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.3%) to 81.827%

Changes Missing Coverage Covered Lines Changed/Added Lines %
iter2/iter2.mbt 0 6 0.0%
stringbuilder/stringbuilder.mbt 0 8 0.0%
set/set.mbt 0 22 0.0%
map/map.mbt 0 23 0.0%
iter/iter.mbt 0 34 0.0%
Totals Coverage Status
Change from base Build 4812: -1.3%
Covered Lines: 4872
Relevant Lines: 5954

💛 - Coveralls

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