Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert changes made in pr #537 to change DataTransfer.data typehint. #570

Conversation

alexmclarty
Copy link
Contributor

According to this issue, a PR was made to change the type of DataTransfer.data to Any rather than str.

This PR makes the type str instead of Any.

…ta typehint. The typehint should be a str type, rather than Any.
@OrangeTux
Copy link
Collaborator

@mdwcrft Does this PR have any impact on our internal codebase?

@mdwcrft
Copy link
Collaborator

mdwcrft commented Jan 17, 2024

As discussed offline I think this should still be of type Any - but we will clarify with the OCA what their definition of anyType means

@proelke
Copy link
Collaborator

proelke commented Jan 17, 2024

The schema specifies a string here.

@proelke proelke closed this Jan 17, 2024
@proelke proelke reopened this Jan 17, 2024
@Jared-Newell-Mobility
Copy link
Contributor

Jared-Newell-Mobility commented Jan 17, 2024

I would suggest also that we add a comment to explain this complication in the standard in detail - I might prevent it reverted again into the future?

@alexmclarty alexmclarty force-pushed the revert_type_hint_change_for_DataTransfer_data branch from 31a335a to a71fc59 Compare January 17, 2024 17:07
@alexmclarty
Copy link
Contributor Author

@Jared-Newell-Mobility ready for review (once tests pass)

@Jared-Newell-Mobility
Copy link
Contributor

I've check over things again, I don't see a type specified in the schema. There is clearly a discrepancy between the descriptions 1. Optional. Data without specified length or format, in response to request and 2. text, data without specified length or format.

Schema normally trumps; I'm going to raise it with the OCA at minimum this might be cleared up also for others in the next Errata

  "$schema": "http://json-schema.org/draft-06/schema#",
  "$id": "urn:OCPP:Cp:2:2020:3:DataTransferRequest",
  "comment": "OCPP 2.0.1 FINAL",
  "definitions": {
    "CustomDataType": {
      "description": "This class does not get 'AdditionalProperties = false' in the schema generation, so it can be extended with arbitrary JSON properties to allow adding custom data.",
      "javaType": "CustomData",
      "type": "object",
      "properties": {
        "vendorId": {
          "type": "string",
          "maxLength": 255
        }
      },
      "required": [
        "vendorId"
      ]
    }
  },
  "type": "object",
  "additionalProperties": false,
  "properties": {
    "customData": {
      "$ref": "#/definitions/CustomDataType"
    },
    "messageId": {
      "description": "May be used to indicate a specific message or implementation.\r\n",
      "type": "string",
      "maxLength": 50
    },
-----------------
    "data": {
      "description": "Data without specified length or format. This needs to be decided by both parties (Open to implementation).\r\n"
    },
-----------------
    "vendorId": {
      "description": "This identifies the Vendor specific implementation\r\n\r\n",
      "type": "string",
      "maxLength": 255
    }
  },
  "required": [
    "vendorId"
  ]
}```

```{
  "$schema": "http://json-schema.org/draft-06/schema#",
  "$id": "urn:OCPP:Cp:2:2020:3:DataTransferResponse",
  "comment": "OCPP 2.0.1 FINAL",
  "definitions": {
    "CustomDataType": {
      "description": "This class does not get 'AdditionalProperties = false' in the schema generation, so it can be extended with arbitrary JSON properties to allow adding custom data.",
      "javaType": "CustomData",
      "type": "object",
      "properties": {
        "vendorId": {
          "type": "string",
          "maxLength": 255
        }
      },
      "required": [
        "vendorId"
      ]
    },
    "DataTransferStatusEnumType": {
      "description": "This indicates the success or failure of the data transfer.\r\n",
      "javaType": "DataTransferStatusEnum",
      "type": "string",
      "additionalProperties": false,
      "enum": [
        "Accepted",
        "Rejected",
        "UnknownMessageId",
        "UnknownVendorId"
      ]
    },
    "StatusInfoType": {
      "description": "Element providing more information about the status.\r\n",
      "javaType": "StatusInfo",
      "type": "object",
      "additionalProperties": false,
      "properties": {
        "customData": {
          "$ref": "#/definitions/CustomDataType"
        },
        "reasonCode": {
          "description": "A predefined code for the reason why the status is returned in this response. The string is case-insensitive.\r\n",
          "type": "string",
          "maxLength": 20
        },
        "additionalInfo": {
          "description": "Additional text to provide detailed information.\r\n",
          "type": "string",
          "maxLength": 512
        }
      },
      "required": [
        "reasonCode"
      ]
    }
  },
  "type": "object",
  "additionalProperties": false,
  "properties": {
    "customData": {
      "$ref": "#/definitions/CustomDataType"
    },
    "status": {
      "$ref": "#/definitions/DataTransferStatusEnumType"
    },
    "statusInfo": {
      "$ref": "#/definitions/StatusInfoType"
    },
-------------------------
    "data": {
      "description": "Data without specified length or format, in response to request.\r\n"
    }
-------------------------
  },
  "required": [
    "status"
  ]
}```

@Jared-Newell-Mobility
Copy link
Contributor

The OCA has confirmed that AnyType is any type as expected and is reflected in the schema. However, in OCPP 2.0.1 Part 2, Section 2.1.4. Primitive Datatypes the description for AnyType could be improved on, that is to remove the word "Text". This has been raised. So given this, I will close this PR as the current type used is correct.

@OrangeTux
Copy link
Collaborator

Thanks for your effort @Jared-Newell-Mobility

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants