FreeOpcUa / opcua-asyncio

OPC UA library for python >= 3.7
GNU Lesser General Public License v3.0
1.11k stars 358 forks source link

Fix #1634 #1635

Closed dakotahorstman closed 5 months ago

dakotahorstman commented 5 months ago

Also:

Note: The following tests did not work prior to this PR on my local computer:

oroulet commented 5 months ago

Generally it would be really great if you would not mix real changes with formatting changes, it makes code review almost impossible. Also are you sure about None? as far as I remember None is how NoSecurity is mentionned everywhere in spec. I agree it is more precise though...

alexrudd2 commented 5 months ago

Note: The following tests did not work prior to this PR on my local computer:

* `FAILED tests/test_server.py::test_runTest - assert None == 123`

* `FAILED tests/test_xml.py::test_xml_import_companion_specifications[client] - FileNotFoundError: [Errno 2] No such file or directory: '/home/user/Documents/opcua-asyncio/nodeset/DI/Opc.Ua.Di.NodeSet2.xml'`

* `FAILED tests/test_xml.py::test_xml_import_companion_specifications[server] - FileNotFoundError: [Errno 2] No such file or directory: '/home/user/Documents/opcua-asyncio/nodeset/DI/Opc.Ua.Di.NodeSet2.xml'`

This happens to me as well, and I think it's because nodeset is a git submodule. What happens if you run below in repo root? git submodule update --init

dakotahorstman commented 5 months ago

I changed "None" to "NoSecurity" to align with the attributes of the enum, along with the removal of underscores of others in the docstring. If the spec outlines "None", then would it be better to note the name discrepancy (since None is a reserved name in Python)? I've never had an issue keeping them straight, it just caught my eye.

I can undo the format changes if you'd like.

dakotahorstman commented 5 months ago

Note: The following tests did not work prior to this PR on my local computer:

* `FAILED tests/test_server.py::test_runTest - assert None == 123`

* `FAILED tests/test_xml.py::test_xml_import_companion_specifications[client] - FileNotFoundError: [Errno 2] No such file or directory: '/home/user/Documents/opcua-asyncio/nodeset/DI/Opc.Ua.Di.NodeSet2.xml'`

* `FAILED tests/test_xml.py::test_xml_import_companion_specifications[server] - FileNotFoundError: [Errno 2] No such file or directory: '/home/user/Documents/opcua-asyncio/nodeset/DI/Opc.Ua.Di.NodeSet2.xml'`

This happens to me as well, and I think it's because nodeset is a git submodule. What happens if you run below in repo root? git submodule update --init

This fixed two of the tests, but test_runTest is still throwing an assertion error same as before. I appreciate the correction :)

oroulet commented 5 months ago

formatting changes are fine, but it makes reading diff much easier of they are in their own commits

oroulet commented 5 months ago

Also I am afraid renaming None will break existing code, so I am not sure we can do that

dakotahorstman commented 5 months ago

Sorry, let me clarify.

All I mean is something like this:

class SecurityPolicyType(IntEnum):
    """
    The supported types of SecurityPolicy.

    Note: The OPC spec uses the keyword "None" to refer to a
    non-signed, non-encrypted endpoint. Because "None" is a reserved
    Python name, we've used "NoSecurity" in its stead. In other words,
    "NoSecurity" here is the same as "None" in the OPC spec.

    "NoSecurity"
    "Basic128Rsa15_Sign"
    "Basic128Rsa15_SignAndEncrypt"
    "Basic256_Sign"
    "Basic256_SignAndEncrypt"
    "Basic256Sha256_Sign"
    "Basic256Sha256_SignAndEncrypt"
    "Aes128Sha256RsaOaep_Sign"
    "Aes128Sha256RsaOaep_SignAndEncrypt"
    "Aes256Sha256RsaPss_Sign"
    "Aes256Sha256RsaPss_SignAndEncrypt"
    """

    NoSecurity = 0
    ...

There are no functional differences, only changes to the docstring.