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

fixes #21 class side methods are not really nice #26

Conversation

n1toxyz
Copy link
Contributor

@n1toxyz n1toxyz commented Mar 24, 2024

This PR refactors the class side methods of CTNewArray2D and its subclasses.

n1toxyz added 5 commits March 24, 2024 22:52
…creation methods.

This change exracts the default implementation class (CTNewArray2DRowsAndColumns) into a class method to provide a hook for subclasses.
Removes hardcoded default implementation class from class methods not related to creating a specific implementation.
…ve unneeded new class method.

This change makes use of the previously implemented hook to reuse generic instance creation methods from CTNewArray2D.
…ed width:height: class method.

This change makes use of the previously implemented hook to reuse generic instance creation methods from CTNewArray2D.
This change adds the tabulate method, which allows to populate the fields of the CTNewArray. This change also refactors the CTNewArray2D class>>width:height:tabulate: to use the new method.
The previous example code was faulty and did not produce the same results on evaluation.
@coveralls
Copy link

Pull Request Test Coverage Report for Build 8412687712

Details

  • 22 of 23 (95.65%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.5%) to 79.764%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Containers-Array2D/CTNewArray2D.class.st 17 18 94.44%
Totals Coverage Status
Change from base Build 8293427799: 0.5%
Covered Lines: 812
Relevant Lines: 1018

💛 - Coveralls

@Ducasse
Copy link
Collaborator

Ducasse commented Mar 25, 2024

Thanks this is cool to have another view on what we did. I was not really satisfied.

@Ducasse
Copy link
Collaborator

Ducasse commented Mar 25, 2024

@Enzo-Demeulenaere this may impact myg because there is no tag so may be we will have to just deprecate the API.

@Ducasse Ducasse merged commit a61126e into pharo-containers:master Mar 25, 2024
2 of 4 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