Attention is currently required from: dexter. laforge has posted comments on this change by dexter. ( https://gerrit.osmocom.org/c/pysim/+/38195?usp=email ) Change subject: filesystem: pass total_len to construct of when encoding file contents ...................................................................... Patch Set 5: (3 comments) File pySim/filesystem.py: https://gerrit.osmocom.org/c/pysim/+/38195/comment/784f5468_9ad6295d?usp=em… : PS5, Line 746: __get_size so this function now actually doesn't return the size, but it returns a dict with a single element `{'total_len': xxx}`. This is a) odd API design. I would expect suhc a method to return just the `Optional[int]`, i.e. an integer or None b) down below you actually use it with named arguments like return `method(..., total_len = self.__get_size()` This means you are also passing a dict and not an integer to that argument... https://gerrit.osmocom.org/c/pysim/+/38195/comment/81646b5e_437ea613?usp=em… : PS5, Line 1343: def _encode_bin(self, abstract_data, total_len: int = None, **kwargs) -> bytes: this also doesn't really match the code above. total_len is *not* receiving an integer value but a dict `{'total_len': Optional[int]}`. File pySim/gsm_r.py: https://gerrit.osmocom.org/c/pysim/+/38195/comment/fad1ca6d_82873551?usp=em… : PS5, Line 293: def _encode_record_bin(self, abstract_data : dict, record_nr : int, **kwargs) -> bytearray: _encode_record_bin now just has **kwargs, while the _encode_bin has a dedicated total_len argument. I think it should be consistent. Either both methods take a **kwargs as new argument (and then have to use `kwargs.get('total_len', None)` to get to the actual value), OR they should both be extended with an explicity `, total_len: Optional[int] = None, **kwargs`. But let's not make them inconsistent. _encode_record_bin should have the same signature as _encode_bin, with the exception that [of course] a record number must be passed to the former and not to the latter. -- To view, visit https://gerrit.osmocom.org/c/pysim/+/38195?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: pysim Gerrit-Branch: master Gerrit-Change-Id: I1b7a51594fbc5d9fe01132c39354a2fa88d53f9b Gerrit-Change-Number: 38195 Gerrit-PatchSet: 5 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Tue, 24 Sep 2024 14:19:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No