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 6: Code-Review-1

(5 comments)

Patchset:

PS6: I think what we see from the several last few iterations of this patchset is that there is a lack of testing. I argue the code of patchset 5 and 6 are broken - yet we do not detect this in unit tests. Please add some tests that exercise those code paths.

File pySim/filesystem.py:

https://gerrit.osmocom.org/c/pysim/+/38195/comment/be057514_7059e409?usp=ema... : PS6, Line 746: def __get_size(self, total_len: Optional[int] = None) -> int: must be `-> Optional[int]` as it also may return `None`

https://gerrit.osmocom.org/c/pysim/+/38195/comment/0884379a_54f82d98?usp=ema... : PS6, Line 1054: def __get_rec_len(self, total_len: Optional[int] = None) -> int: likewise: `-> Optional[int]`

https://gerrit.osmocom.org/c/pysim/+/38195/comment/25bd5ecf_253bd16c?usp=ema... : PS6, Line 1274: here we still don't return an Optional[int] but a dict? I think that's inconsistent comparing the __get_size situation above with the one here for record oriented EF?

https://gerrit.osmocom.org/c/pysim/+/38195/comment/1cf177e7_df86be34?usp=ema... : PS6, Line 1303: and this will again break. Your __get_rec_len() method wil return a dict and you're prefixing that with total_len, so the result is something like `{'total_len': {'total_len': 23}`