randovania / mercury-engine-data-structures

Construct type definitions for Mercury Engine.
MIT License
2 stars 11 forks source link

Write dedicated building code for ArgListSR #179

Closed henriquegemignani closed 4 months ago

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 74.49%. Comparing base (3949b90) to head (d60ea87).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #179 +/- ## ========================================== + Coverage 74.40% 74.49% +0.08% ========================================== Files 65 65 Lines 3243 3254 +11 ========================================== + Hits 2413 2424 +11 Misses 830 830 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

henriquegemignani commented 4 months ago

Still nothing beats reading construct hacks with code in strings :)

I don't think this is a hack. It's specialising the code generation for this struct. I could've gone with a custom class that inherits from DictAdapter and overrides the emitbuild/parse methods, but that's boilerplate for the sake of boilerplate.

Code in strings is the life of doing runtime code generation.

ThanatosGit commented 4 months ago

I consider this a hack because it doesn't really scale very well in terms of readability. If you end up with 10 objects of class A but each has overwritten private methods on an per instance level, it get's really confusing. If they are all subclasses, yeah it's more boiler plate but less confusing when reading the code because you can already see and expect that they do something different because they are instances of different sub classes. An overwritten private method per instance 50 lines later is harder to recognize. Anyways, it's also true that it doesn't matter that much here because the implementation's goal is to lead to the same output eventually.

henriquegemignani commented 4 months ago

In practice, ArgListSR is kinda a class? I think it just reads entirely worse by making a pointless class used in one single place.