Creatures-Developer-Network / prayer

prayer, a <WIP> Python Library for handling PRAY files
MIT License
2 stars 0 forks source link

Refactor to be more pythonic, refactor to use type hinting & py3 types #7

Open pushfoo opened 4 years ago

pushfoo commented 4 years ago

Conversation with @KeyboardInterrupt has made it clear that this is originally a py2 codebase from 2016.

It needs type hinting as the start of cleanup, as well as replacing some of the int parsing with use of the struct module.

pushfoo commented 4 years ago

Currently waiting on feedback on the original design intentions. Recap of the discord messages below. The suggestion below could be punted to a separate ticket as well, with the current one only covering conversion to implement subclass methods _read_body and _write_body.

Recap: At the moment, tag block variables are implemented as a list placed outside the constructor for unclear reasons:

    @staticmethod
    def create_tag_block(block_type, block_name, named_variables):
        tmp_tag_block = TagBlock(Block().block_data)
        tmp_tag_block.type = block_type
        tmp_tag_block.name = block_name
        tmp_tag_block.number_of_integer_variables = 0
        tmp_tag_block.number_of_string_varaibles = 0
        for variable in named_variables:
            if type(variable[1] == int):
                number_of_integer_variables = +1
                tmp_tag_block.named_variables.append(variable)
            elif type(variable[1 == str]):
                number_of_string_varaibles = +1
                tmp_tag_block.named_variables.append(variable)
        return tmp_tag_block

        raise NotImplementedError

This is a fast way of handling multiple entries under the same name, but it's unclear whether c2e supports this edgecase, or if any existing agent files do this. If the engine doesn't support that behavior, it would be more convenient if tag blocks acted as a mapping object rather than having an exposed list attribute. This would allow library users to create custom tags blocks easily from the repl, or modify and inspect existing instances. For example, testing edge cases or strange conditions in SFAM or creature blocks:

>>> tag_block["Creature Age In Ticks"] = 0

It would also be friendlier for library end-users who might want to survey agent files.

Another possibility would be to have strings and ints attributes that present tags separately. It could be implemented as a custom view of a dict. If the engine supports int and string variables with the same name, implementing each table as a separate mapping object may have advantages.

pushfoo commented 4 years ago

I wrote a rough and completely untested version of the tag block that should get the ideas i had in mind across. It imitates existing behavior while being much cleaner. @Ham5ter thoughts?

I think you had the right general idea with the MutableSequence subclass setting a flag when data is added or removed. However, I think that TagBlock itself should be the should be a MutableMapping/MutableSequence subclass. That could give is clean syntax like the following:

>>> tag_block["Creature Age In Ticks"] = 0

However, that should probably be part of another ticket. The following issues should probably be other tickets as well: