409-Resolves problem with loading dependent classes within package(exercise)

exercism / pharo-smalltalk

Exercism exercises in Pharo.

https://exercism.org/tracks/pharo-smalltalk

MIT License

34 stars 28 forks source link

409-Resolves problem with loading dependent classes within package(exercise) #548

Closed Bajger closed 2 years ago

Bajger commented 2 years ago

fixes #409
this will effectively resolve loading exercise, where source files contain dependent classes (one class is subclass of another)
Tonel format used in Pharo uses one file per class for serialization from/to git
When subclass is loaded earlier than superclass, there was a problem with dependency
New way uses MCSnapshot object, that resolves dependencies between class definitions and classes are loaded in proper order

Bajger commented 2 years ago

@glennj : I know it is pretty extensive code change, but this resolves loading dependent classes in same package. I did following:

removed usage of TonelParser and replaced by TonelReader, which put class, method, class extension definitions in right order to be loaded, in the end, reader just uses parser internally
used MCSnapshot to wrap definitions, in case there is a dependency, snapshot resolves that during installation
added more unit tests on submission (which uses memory file reference with sources needed by tonel reader)

Reason: This would allow us to write exercises in more Smalltalk idiomatic way, since we can use multiple classes (that can subclass each other)

It would be great, if you can test review command manually - I'm not the mentor at the moment I will also test manually original code with new code and see if there are differences in download/submit operations (hope not)

Bajger commented 2 years ago

All unit tests green!

Bajger commented 2 years ago

Tested manually download on my solution of robot-simulator (that had dependencies), and tried submit back. Works well.

Bajger commented 2 years ago

@samWson : Hi Sam! I'm not sure how busy you are and if you're in mood to review. Your review is much appreciated!

Bajger commented 2 years ago

@macta, @bencoman I know you guys aren't much involved right now. Kindly asking for review of this, if you have some time. PR addresses one old issue and unifying approach with using Tonel Reader, Writer for loading / serialization of sources.

Bajger commented 2 years ago

@gypsydave5 @glennj : Thank you both for reviews! I've invested quite a time of how to design code import/export capabilities using Tonel classes, so it should be consistent. Prior approach was simpler (since Parser is simpler), but TonelReader can be used as blackbox and takes care about order of loaded definitions. Other element used - MCSnapshot addresses dependenies between classes. Tests are passing and manual testing worked just fine, so it should be fine to merge PR.

Bajger commented 2 years ago

@SleeplessByte Thanks for merging! Failed P9 build is unrelated to these changes. Should be ok to merge.

Bajger commented 2 years ago

@ErikSchierboom Can I ask for merging PR? I'm not currently treated as track maintainer, but I'd be happy to merge PR (my time/effort spent on this) prior new contributor policy is active. If you have any doubts, just let me know :-)