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

adapter: Refactor XML and JSON de-/serialization methods #166

Merged
merged 14 commits into from
Aug 20, 2024

Conversation

zrgt
Copy link
Contributor

@zrgt zrgt commented Nov 15, 2023

The methods for handling attributes of abstract classes seemed to me too big.

In this PR these methods are refactored. Mostly a method extraction refactoring was done, to simplify the methods and to keep less abstraction levels in a single method

The function _expect_type was renamed to _is_of_type as it represents clearer what the func does.

This PR is not relevant for the V3.0 release and can be merged later

Copy link
Contributor

@jkhsjdhjs jkhsjdhjs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's necessary to create a new function for amending HasExtension/HasSemantics/etc. attributes in each (de-)serialization, as they only have one usage each and thus don't aid in reducing duplicate code. I wouldn't mind them for the purpose of shortening the other function though, but that wasn't too long either IMO.
But while you're at it, you can now also put the _amend_*_attrs() functions in a dict with the corresponding type and perform the isinstance() checks in a loop over said dict.

Maybe you can also unify the naming of the amend_*_attributes and extend_with_*_attrs functions to amend_*_attributes, which fits best IMO.

@zrgt
Copy link
Contributor Author

zrgt commented Nov 15, 2023

I don't think it's necessary to create a new function for amending HasExtension/HasSemantics/etc. attributes in each (de-)serialization, as they only have one usage each and thus don't aid in reducing duplicate code. I wouldn't mind them for the purpose of shortening the other function though, but that wasn't too long either IMO.

The purpose of the refactoring is not only reducing duplicated code, but also improving readability.
The function was 50 lines long, which IMO too long. The function had multiple levels of abstraction before, so the readability and further development was affected.

@zrgt
Copy link
Contributor Author

zrgt commented Nov 15, 2023

But while you're at it, you can now also put the _amend_*_attrs() functions in a dict with the corresponding type and perform the isinstance() checks in a loop over said dict.

Good Idea! But may any problems occure because of the order?

@zrgt
Copy link
Contributor Author

zrgt commented Nov 15, 2023

Maybe you can also unify the naming of the amend_*_attributes and extend_with_*_attrs functions to amend_*_attributes, which fits best IMO.

extend_with_*_attrs is used in serialization while in deserialization amend_with_*_attrs is used

@s-heppner s-heppner added enhancement Enhancement of an existing feature v3.0 labels Nov 16, 2023
@s-heppner
Copy link
Contributor

I believe this refactoring made the code more readable and I don't have any strong opinions on the remaining details, so I let you two work them out.

@zrgt zrgt changed the title adapter: Refactor XML and JSON de-/serialization methods [WIP] adapter: Refactor XML and JSON de-/serialization methods Nov 20, 2023
zrgt added 3 commits August 19, 2024 17:13
# Conflicts:
#	basyx/aas/adapter/xml/xml_deserialization.py
Removed calls to abstract method
 "__init__" with trivial body
 via super(), as unsafe
@zrgt zrgt changed the title [WIP] adapter: Refactor XML and JSON de-/serialization methods adapter: Refactor XML and JSON de-/serialization methods Aug 19, 2024
Copy link
Contributor

@s-heppner s-heppner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's discuss the super.__init__() removal, but other than that it LGTM

@s-heppner s-heppner merged commit e40cc34 into eclipse-basyx:v3.0 Aug 20, 2024
6 checks passed
@s-heppner s-heppner deleted the improve/de_serialization branch August 20, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants