Attention is currently required from: fixeria, laforge. dexter has posted comments on this change by dexter. ( https://gerrit.osmocom.org/c/pysim/+/37838?usp=email ) Change subject: pySim-shell: fix CardKeyProvider for chv management commands ...................................................................... Patch Set 4: (2 comments) File pySim-shell.py: https://gerrit.osmocom.org/c/pysim/+/37838/comment/d212fe9a_e72623be?usp=ema... : PS3, Line 868: verify_chv_parser.add_argument('PIN', nargs='?', type=is_decimal,

Why are you moving positional arguments before the optional `--pin-nr`? It's not critical, but makes […] Its the same with verify ADM. I have changed it so that it is consistent with verify_adm. I guess this means I have to go through all parser definitions to see if it is wrong elsewhere.

https://gerrit.osmocom.org/c/pysim/+/37838/comment/6743a4f6_818f9fea?usp=ema... : PS3, Line 902: change_chv_parser.add_argument('NEWPIN', nargs='?', type=is_decimal,

You're breaking compatibility by changing the ordering here: currently it's `OLD NEW`, but your patc […] This is intentional. Since we no longer use the placeholders, we must put NEW as first positional parameter so that we can leave OLD out to in case we want to get OLD from the CardKeyProvider.

-- To view, visit https://gerrit.osmocom.org/c/pysim/+/37838?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: I565b56ac608e801c67ca53d337bdec9efa3f3817 Gerrit-Change-Number: 37838 Gerrit-PatchSet: 4 Gerrit-Owner: dexter <pmaier@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> Gerrit-Reviewer: laforge <laforge@osmocom.org> Gerrit-Attention: laforge <laforge@osmocom.org> Gerrit-Attention: fixeria <vyanitskiy@sysmocom.de> Gerrit-Comment-Date: Thu, 22 Aug 2024 10:00:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria <vyanitskiy@sysmocom.de>