Repository navigation
Combine MiscTablesModule and ExpandedTablesModule - #326
Supremekirb wants to merge 4 commits into
Conversation
PhoenixBound
left a comment
PhoenixBound
left a comment
There was a problem hiding this comment.
We'll need to discuss ideas about the best way to handle "moving" files from one module to another, in the project upgrade process, and pruning out data for redundant modules from the Project.snake. In the meantime, here's what I'd suggest so far.
Thanks again for your work on this.
| # this decorator is used to calculate the free ranges instead of hardcoding them. | ||
| # in 3.9 - 3.11, it's possible to combine @property and @classmethod, so we COULD just do that... | ||
| # however, for the sake for maintainability, a >3.11-style solution is here. | ||
| class classproperty: | ||
| def __init__(self, func): | ||
| self.fget = func | ||
| def __get__(self, instance, owner): | ||
| return self.fget(owner) |
There was a problem hiding this comment.
Unless I'm misunderstanding something, you should be able to combine @staticmethod and @property, instead of using @classmethod.
This is the only spot in CoilSnake where FREE_RANGES is read from:
CoilSnake/coilsnake/ui/common.py
Line 149 in e12aeae
There was a problem hiding this comment.
It's been almost a year so I don't recall the details, but I remember this was giving me a headache. That means I probably landed on this for a reason, so a change would need to be tested
There was a problem hiding this comment.
Something I didn't take into account going into this review was that TABLE_OFFSETS here is a member of the MiscTablesModule class, meaning it might not be as easy to refer to without a self or type(self) or whatever. (You could make TABLE_OFFSETS be a global variable to compensate, but then that would require changing every instance of self.TABLE_OFFSETS in the rest of the code, and probably make Mother 2 support harder down the line.)
A lot of bikeshedding is possible here since the code will probably need to change again once Mother 2 support is added. For now, I went with a simpler solution that doesn't involve any property methods, suggested by Coop. (I just double checked, and the function that initializes FREE_RANGES is only called once, so that's not a concern at least.)
I initially suggested replacing @classmethod with @staticmethod, which would have required additional changes, but Coop pointed out that a simple solution like this was also possible.
This branch has not been deployed
2 participants
This PR combines MiscTablesModule and ExpandedTablesModule by implementing the expansion features of the latter into the former. ExpandedTablesModule no longer exists; the new combination retains the MiscTablesModule name.
Under the hood, things are similar to ExpandedTablesModule, but if no list of pointers to patch is provided, then the table will be considered unexpandable, and regular MiscTablesModule behavior will apply.
Additionally, the FREE_RANGES attribute becomes a property that is calculated at runtime. This means that it is no longer necessary to manually calculate and hardcode the space freed up by expanding a table.
This PR doesn't add expansion capabilities to any new tables. That can come later.