trompamusic / trompace-client

A python library to read from and write to the Trompa CE
Apache License 2.0
1 stars 0 forks source link

AddControlActionObject instead of MergePropertyValueSpecificationPote… #53

Closed aggelosgkiokas closed 3 years ago

aggelosgkiokas commented 3 years ago

AddControlActionObject instead of MergePropertyValue....

pep8speaks commented 3 years ago

Hello @aggelosgkiokas! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 69:13: E265 block comment should start with '# '

Line 104:5: E265 block comment should start with '# ' Line 106:1: E302 expected 2 blank lines, found 1 Line 117:1: E302 expected 2 blank lines, found 1 Line 127:1: E302 expected 2 blank lines, found 1 Line 127:72: E231 missing whitespace after ':' Line 131:24: E231 missing whitespace after ':' Line 139:57: W292 no newline at end of file

Comment last updated at 2020-11-23 15:14:18 UTC
alastair commented 3 years ago

There are two types of mutations for linking two items together. One is Add.... and the other is Merge...., merge will only add once - if it already exists and you add it again, it won't add it a second time. Add will add a relationship multiple times. For that reason we've been using the Merge mutation instead.

In this specific case, can you explain why you changed the name of the mutation, but kept the function name, documentation, and parameters? If you need the ability to make a AddControlActionObject mutation, then we should make a new function for it rather than reusing an existing method. Is MergePropertyValueSpecificationPotentialAction no longer needed? If so, we can delete it

aggelosgkiokas commented 3 years ago

There are two types of mutations for linking two items together. One is Add.... and the other is Merge...., merge will only add once - if it already exists and you add it again, it won't add it a second time. Add will add a relationship multiple times. For that reason we've been using the Merge mutation instead.

In this specific case, can you explain why you changed the name of the mutation, but kept the function name, documentation, and parameters? If you need the ability to make a AddControlActionObject mutation, then we should make a new function for it rather than reusing an existing method. Is MergePropertyValueSpecificationPotentialAction no longer needed? If so, we can delete it

The Merge() mutation seemed not not work. I used the AddControlActionObject from the example in the ce-poc-algorithm repository, where the AddControlActionObject is used. But you are right. In any case I'll create an additional function in the ce-client, and at the end, if the Merge(..) is not needed we can remove it. I'll update the branch and will make a new pull request