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

Adapt constructor of model.Key #82

Merged
merged 7 commits into from
Jun 20, 2023

Conversation

dxvidnrt
Copy link
Contributor

Version 3.0 changes the constructor of model.Key.
This PR implements the changes by:

  • Changing type of parameter _type to model.KeyTypes from model.KeyElements
  • Removing boolean parameter
  • Changing type of parameter value from str to Identifier

Also a mixup of indices in test_base was corrected.

David Niebert added 4 commits April 24, 2023 20:32
Renamed `model.KeyElements` to `model.KeyTypes` as defined for V3.0RC02 and V3.0. Adapted to new constructor of `model.Key`
…test_resolve()`

Multiple ModelReferences had the same Index. Fixed `mypy basyx test` errors
….Identifier(abc)`

V3.0 changed type of `model.Key.value` from `str` to `Identifier`
@dxvidnrt
Copy link
Contributor Author

@s-heppner Ready for merge

@s-heppner
Copy link
Contributor

This seems to not touch model.Key, just the examples and the tests?

Delete unnecessary length check in constructor of `base.Key` because `model.Identifier` checks length constrains
@dxvidnrt
Copy link
Contributor Author

I forgot to change the type of Key/value to model.Identifier, this is fixed now.

@dxvidnrt
Copy link
Contributor Author

@s-heppner
I am wondering if the implementation of __str__ in base.Key is correct or if it should be switched to something like

def __str__(self) -> str:
    return self.value.__str__()

same goes for Key.__repr__ due to value being of type Identifier instead of str.
Else I think the code is ready to merge

@s-heppner
Copy link
Contributor

s-heppner commented Jun 20, 2023

I think __repr__ is fine for the time being.

def __repr__(self) -> str:
        return "Key(type={}, value={})".format(self.type.name, self.value)

Here, the .format()-function would convert self.value to a string, if it wasn't already one.

Nevertheless, I agree, it's better to change __str__ as you described.

__str__() now calls __str__() on value because value is an Identifier object instead of a String
@dxvidnrt
Copy link
Contributor Author

@s-heppner The last commit implements the change in Key.str()

@s-heppner s-heppner merged commit 0136081 into eclipse-basyx:improve/V30 Jun 20, 2023
5 checks passed
@s-heppner s-heppner deleted the V30/BugFixes branch June 20, 2023 17:42
@s-heppner s-heppner added the v3.0 label Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants