mirror of
https://gitea.osmocom.org/sim-card/pysim.git
synced 2026-03-17 02:48:34 +03:00
Update patch set 6
Patch Set 6: (14 comments) Patch-set: 6 CC: Gerrit User 1000028 <1000028@035e6965-6537-41bd-912c-053f3cf69326>
This commit is contained in:
Gerrit User 1000028
committed by
Gerrit Code Review
Gerrit Code Review
parent
50a8e545d7
commit
cf906802e1
242
a65886ca3f8d82f6ec2843babd0cdfaa64b5a700
Normal file
242
a65886ca3f8d82f6ec2843babd0cdfaa64b5a700
Normal file
@@ -0,0 +1,242 @@
|
||||
{
|
||||
"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": 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": "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": 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": "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": "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": "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": "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": "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": 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": 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": "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": "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": 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"
|
||||
}
|
||||
]
|
||||
}
|
||||
Reference in New Issue
Block a user