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=ema... : 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=ema... : 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=ema... : 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.