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.

Sign up for GitHub

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

Jump to bottom

APProcedurePatch: hotfix changing class variables to instance variables #2996

Merged
merged 12 commits into from
Mar 20, 2024
Merged

APProcedurePatch: hotfix changing class variables to instance variables #2996

Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
25e5e68
change class variables to instance variables
Silvris Mar 19, 2024
8620572
Update worlds/Files.py
Silvris Mar 20, 2024
f4ac421
Update worlds/Files.py
Silvris Mar 20, 2024
be970d8
move required_extensions to tuple
Silvris Mar 20, 2024
59fca0b
fix missing tuple ellipsis
Silvris Mar 20, 2024
b15ecd5
fix classvar mixup
Silvris Mar 20, 2024
0cbc81f
rename tokens to _tokens. use hasattr
Silvris Mar 20, 2024
e32bcb1
type hint cleanup
Silvris Mar 20, 2024
3de7b88
Update Files.py
Silvris Mar 20, 2024
3bd2373
Merge remote-tracking branch 'upstream/main' into instruction_patch_h…
Silvris Mar 20, 2024
ef5318e
check using isinstance instead
Silvris Mar 20, 2024
3a90dbb
Merge branch 'main' into instruction_patch_hotfix
Silvris Mar 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions worlds/Files.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ class APProcedurePatch(APAutoPatchInterface):
hash: Optional[str] # base checksum of source file
source_data: bytes
patch_file_ending: str = ""
files: Dict[str, bytes] = {}
files: Dict[str, bytes]

@classmethod
def get_source_data(cls) -> bytes:
Expand All @@ -206,6 +206,7 @@ def get_source_data_with_cache(cls) -> bytes:

def __init__(self, *args: Any, **kwargs: Any):
super(APProcedurePatch, self).__init__(*args, **kwargs)
self.files = {}

def get_manifest(self) -> Dict[str, Any]:
manifest = super(APProcedurePatch, self).get_manifest()
Expand Down Expand Up @@ -301,7 +302,7 @@ class APTokenMixin:
bytes, # WRITE
Tuple[int, int], # COPY, RLE
int # AND_8, OR_8, XOR_8
]]] = []
]]]

def get_token_binary(self) -> bytes:
"""
Expand Down Expand Up @@ -355,6 +356,8 @@ def write_token(self, token_type: APTokenTypes, offset: int, data: Union[bytes,
"""
Stores a token to be used by patching.
"""
if not getattr(self, "tokens", None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I completely forgot about hasattr.
Your getattr(...) would return False for tokens = [] while hasattr(...) would return True. Not sure which is better. getattr could hide mistakes while hasattr is more likely in accidental sharing of class vars again. I feel like hasattr would be expected here though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would agree I think. I mostly used getattr because I forgot hasattr existed.

self.tokens = list()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whu not in __init__ ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand it well, but I was guessing it had something to do with Mixin being in the name, so multiple inheritance is expected, so __init__ is likely to be missed.

I don't understand the intended usage of this class that well though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least in my test using the KDL3 branch, APTokenMixin.__init__ is missed if the subclass is also a ProcedurePatch. Doing it like this means we don't need to reach __init__.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this could be a safer way:

    tokens: Sequence[
        Tuple[APTokenTypes, int, Union[
            bytes,  # WRITE
            Tuple[int, int],  # COPY, RLE
            int  # AND_8, OR_8, XOR_8
        ]]] = ()
    def write_token(self, token_type: APTokenTypes, offset: int, data: Union[bytes, Tuple[int, int], int]):
        """
        Stores a token to be used by patching.
        """
        if not isinstance(self.tokens, list):
            self.tokens = []
        self.tokens.append((token_type, offset, data))

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe safer still:

        if not isinstance(self.tokens, list):
            assert len(self.tokens) == 0, f"some funny business happened with {type(self)}.tokens"
            self.tokens = []

In case someone makes an assignment without write_token
(Should write_token be the only way that tokens gets modified?)

...also maybe renaming it to _tokens

Silvris marked this conversation as resolved.
Show resolved Hide resolved
self.tokens.append((token_type, offset, data))


Expand Down
Loading