catarrh/pysim
1
0
Fork 0
mirror of https://gitea.osmocom.org/sim-card/pysim.git synced 2026-03-16 18:38:32 +03:00
Code Issues Packages Projects Releases Wiki Activity
Files
9fcbd6a9bb39ae2232e51aec48c49771f34c8a42
pysim/a65886ca3f8d82f6ec2843babd0cdfaa64b5a700
Gerrit User 1000005 0bd1474589 Update patch set 7 
Patch Set 7:

(12 comments)

Patch-set: 7
Attention: {"person_ident":"Gerrit User 1000005 \u003c1000005@035e6965-6537-41bd-912c-053f3cf69326\u003e","operation":"REMOVE","reason":"\u003cGERRIT_ACCOUNT_1000005\u003e replied on the change"}
2026-03-15 22:46:47 +00:00

655 lines
21 KiB
Plaintext
Raw Blame History

{
"comments": [
{
"unresolved": false,
"key": {
"uuid": "eb480b1f_7bae1528",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 6
},
"lineNbr": 0,
"author": {
"id": 1000028
},
"writtenOn": "2026-03-04T16:51:51Z",
"side": 1,
"message": "I have looked through this now. Many of the comments are about cosmetic things. I think the only real problem I saw was that the get_next method of RandomHexDigitSource returns a byte array instead of hex digits. I would recommend to add a unit-test to be sure that the various value generators in param_source.py work as expected.",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "12d15f1c_2a6a4de9",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 6
},
"lineNbr": 0,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-10T22:20:49Z",
"side": 1,
"message": "there will be a new patch set soon, thanks for the feedback!",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "8d49fed9_a5e0cf4d",
"filename": "pySim/esim/saip/batch.py",
"patchSetId": 6
},
"lineNbr": 65,
"author": {
"id": 1000028
},
"writtenOn": "2026-03-04T16:51:51Z",
"side": 1,
"message": "why not use [] as default? ...",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "f24a7bae_34d7aa7a",
"filename": "pySim/esim/saip/batch.py",
"patchSetId": 6
},
"lineNbr": 65,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-10T22:20:49Z",
"side": 1,
"message": "IIRC the linter warns against passing empty lists as default argument, because in python there can arise an evil twin situation that is hard to see: like some functions change the arg in-place or something, i don\u0027t remember in detail.",
"parentUuid": "8d49fed9_a5e0cf4d",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "87b50309_cca942f4",
"filename": "pySim/esim/saip/batch.py",
"patchSetId": 6
},
"lineNbr": 65,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-15T22:46:47Z",
"side": 1,
"message": "Done",
"parentUuid": "f24a7bae_34d7aa7a",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "e943de47_0f674ba4",
"filename": "pySim/esim/saip/batch.py",
"patchSetId": 6
},
"lineNbr": 80,
"author": {
"id": 1000028
},
"writtenOn": "2026-03-04T16:51:51Z",
"side": 1,
"message": "... then you could write self.params \u003d params here. Maybe this is easier?",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "bd98a8ed_3caf0795",
"filename": "pySim/esim/saip/batch.py",
"patchSetId": 6
},
"lineNbr": 80,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-10T22:20:49Z",
"side": 1,
"message": "(marking done, bc discussion will be above)",
"parentUuid": "e943de47_0f674ba4",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "e4dbb645_ad12fe16",
"filename": "pySim/esim/saip/batch.py",
"patchSetId": 6
},
"lineNbr": 92,
"author": {
"id": 1000028
},
"writtenOn": "2026-03-04T16:51:51Z",
"side": 1,
"message": "In the above comment you say that self.csv_rows can also be a list. I think next() would not work on lists. Is the comment still up to date? or is something missing here?",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "32c6d044_21645ff6",
"filename": "pySim/esim/saip/batch.py",
"patchSetId": 6
},
"lineNbr": 92,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-10T22:20:49Z",
"side": 1,
"message": "hm that\u0027s right, it has to be an iter(list), thx",
"parentUuid": "e4dbb645_ad12fe16",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "a1278baf_7973bef3",
"filename": "pySim/esim/saip/batch.py",
"patchSetId": 6
},
"lineNbr": 92,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-15T22:46:47Z",
"side": 1,
"message": "Done",
"parentUuid": "32c6d044_21645ff6",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "56c19288_ec4788f0",
"filename": "pySim/esim/saip/batch.py",
"patchSetId": 6
},
"lineNbr": 97,
"author": {
"id": 1000028
},
"writtenOn": "2026-03-04T16:51:51Z",
"side": 1,
"message": "As far as I understand self.csv_rows is an optional parameter? From what I can see, the code should work fine as long as CsvSource is not used in this case.",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "a3e376b3_476ec8e8",
"filename": "pySim/esim/saip/batch.py",
"patchSetId": 6
},
"lineNbr": 97,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-10T22:20:49Z",
"side": 1,
"message": "it is optional, configured by the caller, whether a CSV file is present. If yes, it will pass nonempty csv_rows and use at least one CsvSource. So the csv_rows !\u003d None case definitely has to be supported.",
"parentUuid": "56c19288_ec4788f0",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "ee3801c0_43243e2f",
"filename": "pySim/esim/saip/batch.py",
"patchSetId": 6
},
"lineNbr": 97,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-15T22:46:47Z",
"side": 1,
"message": "Done",
"parentUuid": "a3e376b3_476ec8e8",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "f724aaf0_158e7a7f",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 31,
"author": {
"id": 1000028
},
"writtenOn": "2026-03-04T16:51:51Z",
"side": 1,
"message": "As far as I understand this is an abstract class. Maybe declare it as\n\nclass ParamSource(abc.ABC)\n\n\nBut when I look more closely, this is not really an abstract class. When I get you correctly you want users to be able to use this class as it is as a bare minimum. It wouldn\u0027t do anything useful, but it also wouldn\u0027t crash the application.",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "df80bc66_187a2e6c",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 31,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-10T22:20:49Z",
"side": 1,
"message": "That is right. The ABC discussion is a slightly dangerous one, because I am pretty firm in my opinion against using it, though not everyone seems to agree with it. It would be great if I am allowed to work without ABC, or otherwise let\u0027s schedule a call, or i will hold a talk about ABC -- either way let\u0027s not list points for and against it here but rather talk in person?",
"parentUuid": "f724aaf0_158e7a7f",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "097eacb1_97e92446",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 31,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-15T22:46:47Z",
"side": 1,
"message": "Done",
"parentUuid": "df80bc66_187a2e6c",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "587dd973_a74cc8f0",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 42,
"author": {
"id": 1000028
},
"writtenOn": "2026-03-04T16:51:51Z",
"side": 1,
"message": "If this should work as the bare minimum, we should also have some kind of dummy constructor that can accept the string.",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "b655dbde_252b85c9",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 42,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-10T22:20:49Z",
"side": 1,
"message": "Hmm. it\u0027s even worse, most subclasses completely replace this. I will make a better plan, thanks.",
"parentUuid": "587dd973_a74cc8f0",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "0b078810_199635ae",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 42,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-15T22:46:47Z",
"side": 1,
"message": "Done",
"parentUuid": "b655dbde_252b85c9",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "4eb02f63_a84a2c51",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 51,
"author": {
"id": 1000028
},
"writtenOn": "2026-03-04T16:51:51Z",
"side": 1,
"message": "Excess linebreak? (the other classes are only separated with one)",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "4cd391b0_8a0ba5dd",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 51,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-10T22:20:49Z",
"side": 1,
"message": "yea you\u0027re right, sometimes i do sometimes i don\u0027t, thx.\n\ni think it\u0027s because this separates the base definitions from the actual implementations, but not important",
"parentUuid": "4eb02f63_a84a2c51",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "87a9f9be_2aa2726c",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 51,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-15T22:46:47Z",
"side": 1,
"message": "Done",
"parentUuid": "4cd391b0_8a0ba5dd",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "58bb5fe7_8ecd4faf",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 65,
"author": {
"id": 1000028
},
"writtenOn": "2026-03-04T16:51:51Z",
"side": 1,
"message": "Maybe add type annotations and agree on one distinct type? At the moment this function can accepts anything that can be casted to int. This is fine, but may also be a bit confusing. (see below)",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "5d7a009c_99dc6ec7",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 65,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-15T22:46:47Z",
"side": 1,
"message": "Done",
"parentUuid": "58bb5fe7_8ecd4faf",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "1bada13b_f050cff7",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 80,
"author": {
"id": 1000028
},
"writtenOn": "2026-03-04T16:51:51Z",
"side": 1,
"message": "If this is a private/internal method, I would mark it with double underscore or single underscore?",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "92b2f56b_245598a9",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 80,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-15T22:46:47Z",
"side": 1,
"message": "not intended, in particular, to be internal",
"parentUuid": "1bada13b_f050cff7",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "67ab85f0_e87a96f7",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 95,
"author": {
"id": 1000028
},
"writtenOn": "2026-03-04T16:51:51Z",
"side": 1,
"message": "Here last_value is either an int or it is a string with an integer number. For the constructor this is ok (see above), but it may be confusing still.",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "944bfb95_3efd540d",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 95,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-10T22:20:49Z",
"side": 1,
"message": "heh funny code \u003d) you are right",
"parentUuid": "67ab85f0_e87a96f7",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "ba0fa97e_5d986644",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 95,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-15T22:46:47Z",
"side": 1,
"message": "Done",
"parentUuid": "944bfb95_3efd540d",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "f9f5cb63_dc1aa1d8",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 121,
"author": {
"id": 1000028
},
"writtenOn": "2026-03-04T16:51:51Z",
"side": 1,
"message": "As far as I understand this should return a bytearray but what you actually want is a hexstring. Maybe return b2h(val) instead of val directly?",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "c8a453fc_9b4a59b5",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 121,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-10T22:20:49Z",
"side": 1,
"message": "This uses the fact that all BinaryParameter can take a bytes as input value.\nWe could convert to hexstring and then the BinaryParameter can convert back to binary, but by passing just the bytes we skip the two conversions.\n\nA thought could be this: most places now use a string value to pass parameter settings around, so if we define that a string is the preferred way, then i would change this; but the idea in ConfigurableParameter that I took over from the code I got when I started is that a bytes and BytesIO and hexstring all work, so i would leave the patch unchanged.. agreed?",
"parentUuid": "f9f5cb63_dc1aa1d8",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "691ffc9a_b4ce4220",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 121,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-15T22:46:47Z",
"side": 1,
"message": "Done",
"parentUuid": "c8a453fc_9b4a59b5",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "b44471ae_26d9b7ba",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 149,
"author": {
"id": 1000028
},
"writtenOn": "2026-03-04T16:51:51Z",
"side": 1,
"message": "self.last_value would be easier to read.",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "c9bf8e23_3f3f390f",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 149,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-15T22:46:47Z",
"side": 1,
"message": "Done",
"parentUuid": "b44471ae_26d9b7ba",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "12fdbdf0_b38d9d6d",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 174,
"author": {
"id": 1000028
},
"writtenOn": "2026-03-04T16:51:51Z",
"side": 1,
"message": "You could just do if csv_row: return csv_row.get(self.csv_column) else: raise ...?",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "3a1f2a8f_e471a54c",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 174,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-10T22:20:49Z",
"side": 1,
"message": "it\u0027s more like\n\n if not csv_row or not csv_row.get(self.csv_column):\n raise...\n return csv_row.get(self.csv_column)\n\nso i\u0027m pretty sure this patch as it is is the most minimalistic way of writing this \u003d)",
"parentUuid": "12fdbdf0_b38d9d6d",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "2c36f403_8e1416cd",
"filename": "pySim/esim/saip/param_source.py",
"patchSetId": 6
},
"lineNbr": 174,
"author": {
"id": 1000005
},
"writtenOn": "2026-03-15T22:46:47Z",
"side": 1,
"message": "Done",
"parentUuid": "3a1f2a8f_e471a54c",
"revId": "a65886ca3f8d82f6ec2843babd0cdfaa64b5a700",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
}
]
}
Reference in New Issue View Git Blame Copy Permalink