Combine MiscTablesModule and ExpandedTablesModule#326
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.
| if not patches: continue # non-expandable, doesn't free up space | ||
| table = eb_table_from_offset(offset) | ||
| ranges.append((from_snes_address(offset), from_snes_address(offset+table.size-1))) |
There was a problem hiding this comment.
Less code is needed here if you leave out continue, and it's arguably clearer to read, too:
| if not patches: continue # non-expandable, doesn't free up space | |
| table = eb_table_from_offset(offset) | |
| ranges.append((from_snes_address(offset), from_snes_address(offset+table.size-1))) | |
| if patches: | |
| # A non-empty list here means that the table is moved elsewhere. | |
| # Mark the original table's address range as freeable. | |
| table = eb_table_from_offset(offset) | |
| ranges.append((from_snes_address(offset), from_snes_address(offset+table.size-1))) |
| # 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
| # expanded table - recreate it with our new number of rows | ||
| if self.TABLE_OFFSETS[offset]: | ||
| log.debug("Reading {}.yml as an expanded table".format(table.name.lower())) | ||
| yml_rep = yml_load(f) | ||
| num_rows = len(yml_rep) | ||
| table.recreate(num_rows=num_rows) | ||
| table.from_yml_rep(yml_rep) | ||
| # unexpanded table - standard process | ||
| else: | ||
| table.from_yml_file(f) | ||
|
|
There was a problem hiding this comment.
Nitpick but code comments should apply to the state of the system at the point the comment “executes.” Try moving the comments into the bodies of the if and else:
if self.TABLE_OFFSETS[offset]:
# expanded table - recreate it with our new number of rows
log.debug("Reading {}.yml as an expanded table".format(table.name.lower()))
# ...I think it reads clearer that way
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.