Make diffgraph applier return number of transitive changes #256
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mpollmeier
approved these changes
Sep 16, 2024
| @@ -12,7 +12,7 @@ object DiffGraphApplier { | |||
| graph: Graph, | |||
| diff: DiffGraphBuilder, | |||
| schemaViolationReporter: SchemaViolationReporter = new SchemaViolationReporter | |||
| ): Unit = { | |||
| ): Int = { | |||
There was a problem hiding this comment.
Our future selves are demanding to add some comments like /** returns the number of modifications that were successfully applied to the graph */ to methods like this, which only return an Int that could mean anything or nothing.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With this, the diffgraph applier again counts the number of transitive modifications in the graph, in order to restore the logging.
Furthermore this restores the correct order of nodes that are transitively added -- we need to
drainDeferredafter every single update, not just at the end. This behavior was a little subtle and was missing both tests and a comment explaining it, so it got lost in a refactor (cf git blame). Now it's protected by both source-code comment and unit test.