Skip to content

Refactor: Clean up dead code and align copy with standard Pharo lifecycle#10

Merged
jordanmontt merged 3 commits intopharo-containers:masterfrom
HossamSaberr:master
Mar 20, 2026
Merged

Refactor: Clean up dead code and align copy with standard Pharo lifecycle#10
jordanmontt merged 3 commits intopharo-containers:masterfrom
HossamSaberr:master

Conversation

@HossamSaberr
Copy link
Copy Markdown
Contributor

@HossamSaberr HossamSaberr commented Feb 24, 2026

Fixes #9

@Ducasse
Copy link
Copy Markdown
Collaborator

Ducasse commented Feb 26, 2026

Thanks for your contribution. Can you add a couple of tests checking that the state is not shared?

@HossamSaberr
Copy link
Copy Markdown
Contributor Author

HossamSaberr commented Feb 26, 2026

Thank you for the review, @Ducasse! I've just pushed a new commit.
Added the test and its green!
image

Let me know if you need any further adjustments!

@Ducasse
Copy link
Copy Markdown
Collaborator

Ducasse commented Feb 26, 2026

Tx for this good test!

@HossamSaberr
Copy link
Copy Markdown
Contributor Author

HossamSaberr commented Mar 7, 2026

@Ducasse
just ping you if this PR need any adjustments

jordanmontt
jordanmontt reviewed Mar 18, 2026
View reviewed changes
Comment thread src/Containers-KeyedTree/CTKeyedTree.class.st Outdated
ifFalse: [ assoc value ]) ] ]
super postCopy.
self keysAndValuesDo: [ :key :value |
(value isKindOf: self class)
Copy link
Copy Markdown
Member

@jordanmontt jordanmontt Mar 18, 2026

Choose a reason for hiding this comment

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

why do we need to type check in here? assoc value isKindOf: self class is type checking

Copy link
Copy Markdown
Contributor Author

@HossamSaberr HossamSaberr Mar 18, 2026

Choose a reason for hiding this comment

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

You are right, i carried it over from the old dead postCopy code to ensure I didn't unintentionally break any legacy behavior

ig we can fix that using Polymorphism by Just send #copy to everything: self keysAndValuesDo: [ :key :value | self at: key put: value copy ]

Comment thread src/Containers-KeyedTree-Tests/CTKeyedTreeTest.class.st
@jordanmontt jordanmontt merged commit fcf6c00 into pharo-containers:master Mar 20, 2026
15 checks passed
@coveralls
Copy link
Copy Markdown

coveralls commented Mar 20, 2026 •
edited
Loading

Pull Request Test Coverage Report for Build 23247261395

Details

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.0%) to 65.962%
Totals Coverage Status
Change from base Build 18003962076: 1.0%
Covered Lines: 343
Relevant Lines: 520
💛 - Coveralls

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

Reviewers

@jordanmontt jordanmontt jordanmontt left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Clean up dead code

4 participants

@HossamSaberr @Ducasse @coveralls @jordanmontt