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=em… : 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=em… : 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=em… : 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=em… : 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}` -- 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: 6 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: Thu, 26 Sep 2024 21:33:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes