Support for embedding of single attributes #332
Conversation
trisch-me
changed the title
|
Partial fix for #240 |
| some server id | ||
| examples: ['123'] | ||
| - ref: geo.lat | ||
| prefix: true |
There was a problem hiding this comment.
I believe we previously discussed embed: geo.lat instead of ref: geo.lat and prefix: true.
What would be the reason to not do embed: geo.lat based on the original design and do the prefix flag instead?
Why I'd prefer a new verb (embed)
refalways means the same thing - reference attribute and keep the original name.- obvious when reading. Imagine 10 other lines between
refandprefixand how hard it'd be to understand what name is supposed to be generated - easier to check in policies
prefixfeels like a string property and not a boolean flag.
There was a problem hiding this comment.
we have discussed it few weeks ago while I have started to implement it in weaver, so that ref and embed are the same in functionality and difference is only in prefixing the parent namespace or not. Hence this approach was selected. Embed will stay for the whole namespace.
I have announced this change also in embed issue linked in this PR
There was a problem hiding this comment.
it still has all the problems listed above, so I'd like to revisit this choice
There was a problem hiding this comment.
I can address these problems because IMO they are not critical
- ref means reference and this is also referencing attribute from another namespace. The difference is in prefix of the attribute. So
embedmeansref+prefixof the parent group. And this is how it was implemented - I personally don't feel this way, we do have a lot of things, like for example ref itself can override description etc. They all means something therefore they are written there
- It's additional check for policy (
ref+prefix) instead just (embed), but I don't feel it's something complicated - regarding string property - this is probably due to the fact that we already have such keyword for the group.
The main reason why we went this way was as described before complete similarity between ref and new embed, therefore we have decided to go with it.
I would like to get more opinions on this @jsuereth @lquerel
There was a problem hiding this comment.
So do you agree that these are problems, critical or not, and ideally we would have one-line that expresses that attribute is embedded instead of two?
It'd also be confusing to have ref to embed one attribute and embed to embed all.
If we want to support both, let's find a good option with embed_one + embed_all, embed + embed_all, etc.
I'm struggling to come up with a real-life use-case for "embed-all-attributes from" and I see a lot of places where embed-one would work great. So both of the above options seem reasonable to me.
There was a problem hiding this comment.
we do have already an option to embed all and embed only one. They are written in the original issue. Embeded namespaces will be defined on the top level of the group, while embedded attribute is defined under attributes.
I'm struggling to come up with a real-life use-case for "embed-all-attributes from"
we do have a lot of such real examples. The most prominent one is about geo namespace included into client or server. Then process included into process as parent node and so on.
Similarity in what sense?
In both. On tolling side it has the same implementation with additional prefix. On semantical meaning they also both are references to another namespace with difference in prefix.
Let's discuss it tomorrow during our tooling meeting
There was a problem hiding this comment.
Let's get back to the original concern: having prefix: true under reference has problems (critical or not). We should try to design schema to be easy to use, easy to read, etc. The "not a critical problem" is not a good argument.
Having embed_one and embed_all verbs would result in much better outcome for schema users. So why the pushback?
There was a problem hiding this comment.
As I have stated before, this decision was made due to similarity of ref and embed functionality. I don't see this as a problem otherwise I wouldn't go and implement it.
Therefore I propose to discuss it once more during our meeting
There was a problem hiding this comment.
I do see above problems with this approach, which would be resolved if we used embed_one or similar with no visible downsides.
The embedding is semantically similar to definition of the attribute (as it creates new attribute).
There was a problem hiding this comment.
We have discussed it yesterday and I will propose new ideas after a meeting with @lquerel today
Thank you for raising your concerns here.
update build tools to support embedding of attribute implemented in weaver