-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add DiffEvents class to generate changelog events from a diff #5692
base: develop
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging #5692 will not alter performanceComparing Summary
|
return node_changelog | ||
|
||
def _process_node_attribute(self, node: NodeChangelog, attribute: EnrichedDiffAttribute) -> None: | ||
changelog_attribute = AttributeChangelog(name=attribute.name, kind="Can_this_be_found?") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the kind of an attribute/relationship is not included in the diff right now
changelog_rel: RelationshipCardinalityOneChangelog | RelationshipCardinalityManyChangelog | ||
match relationship.cardinality: | ||
case RelationshipCardinality.ONE: | ||
changelog_rel = RelationshipCardinalityOneChangelog(name=relationship.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a cardinality=one EnrichedRelationshipDiff
will have a single EnrichedDiffSingleRelationship
element in its relationships
field that can be used to get previous/new values
the EnrichedDiffSingleRelationship.properties
field will have EnrichedDiffProperty
s that give the new/previous values for each property type IS_RELATED
/IS_VISIBLE
/etc
|
||
case RelationshipCardinality.MANY: | ||
changelog_rel = RelationshipCardinalityManyChangelog(name=relationship.name) | ||
for peer in relationship.relationships: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the peer
here is an EnrichedDiffSingleRelationship
that will have a peer_id
value that will be set to the new/current peer ID (or if the previous peer ID if the relationship has been deleted)
5683401
to
153acdc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few comments, but looks solid to me
I'm confident that I will be able to approve it tomorrow before I'm out
|
||
def _populate_diff_nodes(self) -> None: | ||
self._diff_nodes = { | ||
node.uuid: NodeInDiff(node_id=node.uuid, kind=node.kind, label=node.label) for node in self._diff.nodes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could filter out nodes where node.action is DiffAction.UNCHANGED
here instead of in collect_changelogs
it seems like you'll never care about an UNCHANGED
node in this context
self._process_node_attribute(node=node_changelog, attribute=attribute, schema=schema) | ||
|
||
for relationship in node.relationships: | ||
self._process_node_relationship(node=node_changelog, relationship=relationship) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can also filter for action is DiffAction.UNCHANGED
at the attribute, relationship, and relationship element levels. these won't come up too frequently b/c it requires someone to manually revert a change on a branch, but it will still happen occasionally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks great so far
sadly, there is one extra complication that you'll have to account for: diff conflicts. BUT! I think you'll only need to add a few conditionals in here to handle them, so not so bad
conflicts can exist on a node EnrichedDiffNode
, a relationship element EnrichedDiffSingleRelationship
, or a property EnrichedDiffProperty
at this point in the logic (the merge has already been done), every conflict must have been resolved one way or the other.
- if a conflict was resolved to select the base/target branch, then you can just ignore the item b/c the merge process left it unchanged on
main
. - if the conflict was resolved to select the diff/source branch, then you just proceed as the code currently works
so I think you just need to add a few lines like
if node.conflict and node.conflict.conflict_selection is ConflictSelection.BASE_BRANCH:
continue
153acdc
to
a98234b
Compare
This PR introduces a changelog DiffEvents class that can be used to generate NodeChangeLogs from a branch diff. The goal is to use these objects to send node mutation events after a branch has been merged. In this PR I've only added the logic to generate these objects, the next PR will contain the logic to actually start sending them.
As part of this I'm also modifying the merger so that it returns a diff object.
Other upcoming work: