Closed jamesreprise closed 1 year ago
Do not edit the content of this comment. The PR reviewer should simply update this comment by ticking each review item below, as they get completed.
Trivial Change
Code
Architecture
Does it make sense for the responsibility of adding vertices to the graph to be split between GraphArea
and GraphBuilder
? I think it makes more sense to move all functionality regarding the management of graph vertices into GraphBuilder
.
My reasoning for this is that once we move the logic for conducting explanations into GraphBuilder
, GraphArea
has no need for any reference to the transaction at all. This seems cleaner architecturally. If we follow this route, perhaps we can rename GraphBuilder
to GraphManager
to reflect that it is responsible for the entire life of the graph rather than just its creation.
The intent was always that GraphBuilder
would be solely responsible for constructing the graph structure based on query answers, but then adding in the Explain functionality started to muddy the waters when it came to domains of responsibility.
Yes, it would make sense for all logic that modifies the graph structure to be concentrated into a single class. Not sure about GraphManager
as a name though, I think we can come up with something more precise.
What is the goal of this PR?
We have provided a friendly warning to users when they try to explain an inference while explanations are not enabled.
Closes https://github.com/vaticle/typedb-studio/issues/608
What are the changes implemented in this PR?
We have a new warning message popup, informing users that they must enable explanations under the necessary circumstances.
To do so:
GraphArea
andGraphBuilder
, removing it as a property fromGraphArea
and making all changes to the graph throughGraphBuilder
.GraphBuilder
calledtransactionSnapshot
. This is a way to use the transaction that was used to create the graph, but only if snapshot was enabled at the time the graph was created and the currenttransactionState.transaction
is the same transaction as was used to create the graph.TransactionState
exposes whether a configuration option like 'explain' is enabled for the given transaction or not. Contrary to what one might assume,transactionState.explain.enabled
does not mean that explain is enabled (which is whattransactionState.explain.value
does) - it means that the explain option could be toggled through the UI. We've renamedenabled
tocanToggle
in this context.