Attempt to solve issue with Duplicating composite instance names imported from different namespaces#95
Conversation
🚨 BugBot failed to runRemote branch not found for this Pull Request. It may have been merged or deleted (requestId: serverGenReqId_d8783b37-101c-4a37-844a-7fe19135a6e1). |
🚨 BugBot failed to runRemote branch not found for this Pull Request. It may have been merged or deleted (requestId: serverGenReqId_7f17ebb8-4dd5-4499-9ec6-cd68ebcb0e2f). |
* added depth field to composite token and passed it to name extraction function * on collecting tokens added step to calculate if there are collisions and increase depth for tokens with such names, additionally register aliases if those do not exist Issue: cartridge-gg#90
baitcode
force-pushed
the
bugfix-event-conflicts
branch
from
238a5cf to
ff63529
Compare
🚨 BugBot failed to runRemote branch not found for this Pull Request. It may have been merged or deleted (requestId: serverGenReqId_3eabd943-d71f-4928-bd06-1ba49c22f57e). |
🚨 BugBot failed to runRemote branch not found for this Pull Request. It may have been merged or deleted (requestId: serverGenReqId_02ff0cca-d2fa-4cf7-84ae-338a69cc0eda). |
🚨 BugBot failed to runRemote branch not found for this Pull Request. It may have been merged or deleted (requestId: serverGenReqId_19303be5-7ac6-472d-8197-a21fbf8c23fa). |
🚨 BugBot failed to runRemote branch not found for this Pull Request. It may have been merged or deleted (requestId: serverGenReqId_fa1a11b2-e13e-431a-a26b-eeb727350c61). |
🚨 BugBot failed to runRemote branch not found for this Pull Request. It may have been merged or deleted (requestId: serverGenReqId_cbd1b46f-53af-4f0f-9310-bc14a3c7017d). |
🚨 BugBot failed to runRemote branch not found for this Pull Request. It may have been merged or deleted (requestId: serverGenReqId_9be3c461-293a-49f7-bf08-67398b58d455). |
glihm
left a comment
glihm
left a comment
There was a problem hiding this comment.
Thank you @baitcode for this proposal.
Reviewing, I would say it is something that could work in most cases, since when it comes to components, the parent's path of the Event is usually different enough to work.
Example with the ERC20 you've shared OOB:
"openzeppelin::access::ownable::ownable::OwnableComponent::Event"
-> "OwnableComponentEvent"
// ...
"openzeppelin::presets::erc20::ERC20::Event"
-> "ERC20Event"
However, the point of types_aliases was to avoid making automatic decisions where naming could be complicated to guess for the end user. Even if, with proper documentation, it should be straightforward to find.
Maybe a middle ground could be to expose a parameter to toggle on-off this auto-renaming. Like so the user could have more control on the generated output, WDYT?
Or even further, one could define the depth of the auto-renaming/auto-resolving of conflicts.
0 -> nothing is done automatically, only user-defined type aliases.
1 -> the implementation you proposed here.
n -> let the user define how many path segments should be used to resolve types collisions.
| // In theory this is not enough, as after deepening collisions are still possible | ||
| // Although possible in theory should not be a problem in practice. |
There was a problem hiding this comment.
This comment could be re-written to be more instructive on why this loop is done.
Loops through the tokens to identify naming collisions at the type name level.
| // Crotch to handle cases when type with the same typename are used | ||
| // in same contract. For example: | ||
| // enum Event { | ||
| // Event1(namespace1::Event), | ||
| // Event2(namespace2::Event) | ||
| // } | ||
| // When name that occures several times is spotted we register a type alias | ||
| // (only if there is none). Will apply those later. |
There was a problem hiding this comment.
Same here, you should also provide the output of your example.
You could actually refactor this logic into a function (including the one looping on the tokens to get the name occurence count).
Like so, if the proposition of making this depth dynamic sounds good to you, we could avoid this extra loop when the user doesn't want cainome to auto-resolve the conflicts.
You could like so provide the example + the detail of the behavior in the function documentation. 👍
| t.apply_alias(type_path, alias); | ||
| } | ||
|
|
||
| // NOTE: it's important that user defined aliases were applied first |
There was a problem hiding this comment.
Interesting note, but you didn't explain why. :)
2 participants
I'm checking if name collision exist and register a type alias (if there is none already)
Issue #90