Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
pushfoo opened this issue Apr 15, 2020 · 2 comments
Open
Assignees

Comments

@pushfoo
Copy link
Member

pushfoo commented Apr 15, 2020

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 pushfoo self-assigned this Apr 15, 2020
@pushfoo pushfoo changed the title Add type hinting & fix py2 types Refactor to be more pythonic, refactor to use type hinting & py3 types May 26, 2020
@pushfoo
Copy link
Member Author

pushfoo commented May 26, 2020

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
Copy link
Member Author

pushfoo commented May 26, 2020

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:

  • invalidating the body caches (related to sequence/mapping subclass due to __setitem__ dependency)
  • general stream support enhancement, such as.write_to and .read_from as static methods

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants