search

openrsgis / pytdml

Python library for TrainingDML-AI encode/decode
MIT License
16 stars 7 forks source link

Work towards working tests #17

Open photonbit opened 2 months ago

photonbit commented 2 months ago

I made this pull request from the work of making the tests pass.

The work in this pull request is outdated after https://github.com/openrsgis/pytdml/pull/15 was merged.

The changes on that pull request are really difficult to grasp, as the code was moved and heavily modified in a single commit.

The present pull request includes items that I think should be included in the mainstream classes.

Pydantic V2 compliance

Pydantic V2 way of configuring the class is using a model_config of type ConfigDict, instead of defining a Config class.

It also changes the way to serialize and de-serialize the models, using model_dump instead of dict, for example, or using min_length instead of min_items

Extended base class functionality

Some io functionality was added to the base class, like reading from a yaml or json file, handling exclude_none and exclude_none on model_dump

Class properties harmonization

Some of the properties were written using camel case and others snake case. The base type allows to define the properties in snake case, as it is customary in python, while at the same time being able to both read and write the io operations correctly according to the standard, that is in camel case.

The proposal is to use snake case in all the properties, allowing the alias_generator = to_camel of the base class to operate.

Default values

Most of the Optional properties did not have a default value, which is the cause of the point 2 in the issue https://github.com/openrsgis/pytdml/issues/11

Other optional values had a default of a list, an empty string or a minimum number of elements, while they should be initialized with None and later automatically remove the properties with None values when serializing.

Incorrect / incomplete type definition

Some types were updated to comply with the 1.0 encoding, I see some of those updated in the all_types, but I am not sure about all the changes, as the new work is in a new file with a different order.

I am opening this pull request as a Draft, because it is meant to start a conversation and not to be merged right away.

Relifest commented 2 months ago

Based on your suggestion, I have summarized the following points that need to be modified:

  1. Pydantic V2 compliance：Pydantic V2 way of configuring the class is using a model_config of type ConfigDict, instead of defining a Config class.
  2. Class properties harmonization: The proposal is to use snake case in all the properties, allowing the alias_generator = to_camel of the base class to operate. I will make modifications based on your suggestions.
  3. Default values: I have set default values for all optional parameters in all_types
  4. Incorrect / incomplete type definition: I will pay attention to checking the definition of the class in the future.
  5. Separate the code and the example files.
  6. Restore to the original basic_types and extended-types modes to prevent damage to backward compatibility. I will make revisions to each of these issues in the future. Thank you for your valuable suggestions.
photonbit commented 2 months ago

Based on your suggestion, I have summarized the following points that need to be modified:

  1. Pydantic V2 compliance：Pydantic V2 way of configuring the class is using a model_config of type ConfigDict, instead of defining a Config class.
  2. Class properties harmonization: The proposal is to use snake case in all the properties, allowing the alias_generator = to_camel of the base class to operate. I will make modifications based on your suggestions.
  3. Default values: I have set default values for all optional parameters in all_types
  4. Incorrect / incomplete type definition: I will pay attention to checking the definition of the class in the future.
  5. Separate the code and the example files.
  6. Restore to the original basic_types and extended-types modes to prevent damage to backward compatibility. I will make revisions to each of these issues in the future. Thank you for your valuable suggestions.

Hello!

From all the mentioned ones, the 6. is the one it might be best just to continue with the all_types approach and at a later stage make the division in different files. The mention about backwards compatibility was something to take into account in the future, but at the current stage is not that important as the library was in a non installable state in previous versions.

I can happily help with any of the tasks :smile_cat: