Skip to content

UX improvements to transaction commits and concept deletion #178

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Nov 2, 2021
Merged

UX improvements to transaction commits and concept deletion #178

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
18d4445
Print the concepts that are being deleted
Oct 29, 2021
d4d3aed
Indicate uncommitted changes
Oct 29, 2021
21fe2fb
bump
Nov 1, 2021
ba2918c
Fixes as per review
Nov 2, 2021
7103f21
More fixes
Nov 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 37 additions & 7 deletions TypeDBConsole.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Consumer;
import java.util.stream.Stream;

Expand All @@ -102,6 +104,7 @@ public class TypeDBConsole {
private final Printer printer;
private ExecutorService executorService;
private Terminal terminal;
private boolean hasUncommittedChanges = false;

private TypeDBConsole(Printer printer) {
this.printer = printer;
Expand Down Expand Up @@ -275,16 +278,17 @@ private boolean transactionREPL(TypeDBClient client, String database, TypeDBSess
.terminal(terminal)
.variable(LineReader.HISTORY_FILE, TRANSACTION_HISTORY_FILE)
.build();
StringBuilder prompt = new StringBuilder(database + "::" + sessionType.name().toLowerCase() + "::" + transactionType.name().toLowerCase());
StringBuilder promptBuilder = new StringBuilder(database + "::" + sessionType.name().toLowerCase() + "::" + transactionType.name().toLowerCase());
if (options.isCluster() && options.asCluster().readAnyReplica().isPresent() && options.asCluster().readAnyReplica().get())
prompt.append("[any-replica]");
prompt.append("> ");
promptBuilder.append("[any-replica]");
try (TypeDBSession session = client.session(database, sessionType, options);
TypeDBTransaction tx = session.transaction(transactionType, options)) {
hasUncommittedChanges = false;
while (true) {
Either<TransactionREPLCommand, String> command;
try {
command = TransactionREPLCommand.readCommand(reader, prompt.toString());
String prompt = hasUncommittedChanges ? promptBuilder + "*> " : promptBuilder + "> ";
command = TransactionREPLCommand.readCommand(reader, prompt);
} catch (InterruptedException e) {
break;
}
Expand All @@ -309,6 +313,7 @@ private boolean transactionREPL(TypeDBClient client, String database, TypeDBSess
break;
} else if (replCommand.isSource()) {
runSource(tx, replCommand.asSource().file(), replCommand.asSource().printAnswers());
hasUncommittedChanges = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eh? what if the file only has match queries, then there's not uncommitted right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'd assumed it was hard to verify that condition, but on second inspection we can do it.

} else if (replCommand.isQuery()) {
runQueriesPrintAnswers(tx, replCommand.asQuery().query());
}
Expand Down Expand Up @@ -651,18 +656,43 @@ private void runQueryPrintAnswers(TypeDBTransaction tx, TypeQLQuery query) {
if (query instanceof TypeQLDefine) {
tx.query().define(query.asDefine()).get();
printer.info("Concepts have been defined");
hasUncommittedChanges = true;
} else if (query instanceof TypeQLUndefine) {
tx.query().undefine(query.asUndefine()).get();
printer.info("Concepts have been undefined");
hasUncommittedChanges = true;
} else if (query instanceof TypeQLInsert) {
Stream<ConceptMap> result = tx.query().insert(query.asInsert());
printCancellableResult(result, x -> printer.conceptMap(x, tx));
AtomicBoolean changed = new AtomicBoolean(false);
printCancellableResult(result, x -> {
changed.set(true);
printer.conceptMap(x, tx);
});
if (changed.get()) hasUncommittedChanges = true;
} else if (query instanceof TypeQLDelete) {
tx.query().delete(query.asDelete()).get();
printer.info("Concepts have been deleted");
Stream<ConceptMap> result = tx.query().match(query.asDelete().match());
AtomicInteger answerCount = new AtomicInteger();
printCancellableResult(result, x -> {
answerCount.getAndIncrement();
printer.conceptMap(x, tx);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is odd - we've introduce an strong inconsistency with our protocol, rather than a minor one, by printing out all the maps that are being deleted. The user doesn't really have any interested in the concepts from which is deleted, so we shouldn't print them (they may not exist in a bit anyway!). Using this to count is ok though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that's fine. Seems odd to do all the work just to throw it away in the end, but given that it's not in line with the eventual solution we're aiming for, we probably shouldn't introduce unrealistic user expectations by printing them now.

});
if (answerCount.get() > 0) {
tx.query().delete(query.asDelete()).get();
printer.info("Concepts have been deleted");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should just change this to Deleted from <count> matched answers

hasUncommittedChanges = true;
} else {
printer.info("No concepts were matched");
}
} else if (query instanceof TypeQLUpdate) {
Stream<ConceptMap> matchResult = tx.query().match(query.asUpdate().match());
AtomicInteger answerCount = new AtomicInteger();
printCancellableResult(matchResult, x -> {
answerCount.getAndIncrement();
printer.conceptMap(x, tx);
});
Stream<ConceptMap> result = tx.query().update(query.asUpdate());
printCancellableResult(result, x -> printer.conceptMap(x, tx));
if (answerCount.get() > 0) hasUncommittedChanges = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aren't we going to print everything twice now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes. image

} else if (query instanceof TypeQLMatch) {
Stream<ConceptMap> result = tx.query().match(query.asMatch());
printCancellableResult(result, x -> printer.conceptMap(x, tx));
Expand Down