Skip to content
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

feat: improve commands_extension #1937

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+365 −96
Open

feat: improve commands_extension #1937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3bdbe11
feat: improve command_extension
TheOneWithTheBraid Oct 12, 2024
afa719a
chore: use stdout for default command output
TheOneWithTheBraid Oct 14, 2024
3fc0164
feat: add non-room related command_runner tests
TheOneWithTheBraid Nov 7, 2024
06ba0c4
fix: commands test
TheOneWithTheBraid Dec 13, 2024
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
29 changes: 23 additions & 6 deletions lib/fake_matrix_api.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@ class FakeMatrixApi extends BaseClient {

static Map<String, List<dynamic>> get calledEndpoints =>
currentApi!._calledEndpoints;

static int get eventCounter => currentApi!._eventCounter;

static set eventCounter(int c) {
currentApi!._eventCounter = c;
}
Expand All @@ -67,6 +69,8 @@ class FakeMatrixApi extends BaseClient {
bool _trace = false;
final _apiCallStream = StreamController<String>.broadcast();

static RoomsUpdate? _pendingRoomsUpdate;

static FakeMatrixApi? currentApi;

static Future<String> firstWhereValue(String value) {
Expand Down Expand Up @@ -105,12 +109,13 @@ class FakeMatrixApi extends BaseClient {
'${request.url.path.split('/_matrix').last}?${request.url.query}';
}

// ignore: avoid_print
if (_trace) print('called $action');

if (action.endsWith('?')) {
action = action.substring(0, action.length - 1);
}

// ignore: avoid_print
if (_trace) print('called $action');

if (action.endsWith('?server_name')) {
// This can be removed after matrix_api_lite is released with:
// https://gitlab.com/famedly/libraries/matrix_api_lite/-/merge_requests/16
Expand Down Expand Up @@ -214,6 +219,8 @@ class FakeMatrixApi extends BaseClient {
'curve25519': 10,
'signed_curve25519': 100,
},
if (_pendingRoomsUpdate != null)
'rooms': _pendingRoomsUpdate?.toJson(),
};
} else if (method == 'PUT' &&
_client != null &&
Expand Down Expand Up @@ -2537,9 +2544,19 @@ class FakeMatrixApi extends BaseClient {
'/client/v3/pushers/set': (var reqI) => {},
'/client/v3/join/1234': (var reqI) => {'room_id': '1234'},
'/client/v3/logout/all': (var reqI) => {},
'/client/v3/createRoom': (var reqI) => {
'room_id': '!1234:fakeServer.notExisting',
},
'/client/v3/createRoom': (var reqI) {
final roomId = '!1234:fakeServer.notExisting';
unawaited(
Future.delayed(Duration(milliseconds: 100)).then((_) {
_pendingRoomsUpdate =
RoomsUpdate(join: {roomId: JoinedRoomUpdate()});
}),
);

return {
'room_id': roomId,
};
},
'/client/v3/rooms/!localpart%3Aserver.abc/read_markers': (var reqI) => {},
'/client/v3/rooms/!localpart:server.abc/kick': (var reqI) => {},
'/client/v3/rooms/!localpart%3Aserver.abc/ban': (var reqI) => {},
Expand Down
2 changes: 1 addition & 1 deletion lib/src/client.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ class Client extends MatrixApi {
DateTime? _accessTokenExpiresAt;

// For CommandsClientExtension
final Map<String, FutureOr<String?> Function(CommandArgs)> commands = {};
final Map<String, CommandExecutionCallback> commands = {};
nico-famedly marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also pls add "BREAKING" in the "feat: improve command_extension" commit message?

final Filter syncFilter;

final NativeImplementations nativeImplementations;
Expand Down
2 changes: 2 additions & 0 deletions lib/src/room.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,6 +624,7 @@ class Room {
String msgtype = MessageTypes.Text,
String? threadRootEventId,
String? threadLastEventId,
StringBuffer? commandStdout,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a little bit dartdoc comments to make it more intuitive how to use the StringBuffer? Or at least a link to the documentation. For example I see this type for the first time and would have to look it up before using it.

Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to have more information from the output as just a String, as errors could also be interesting, while we also might get errors and values at the same time. Could be probably solved by introducing a CommandResult type which contains the success data, (Matrix-)errors as well as the output of the command

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmmm, in my implementation I simply used the regular error catching around the Client.parseAndRunCommand() and Room.send* methods for error handling. Since there are decent, unified Error types available I don't see an advantage in a dedicated error collection mechanism ; but if you have any proposal on how to implement some improved error handling, I'm curious for suggestions !

The advantage of a StringBuffer is that it basically allows to perform random access (similar to a File.open call) and object-oriented append syntax. I simply considered this as way more convenient for this use case - since a String could easily accidentally lose it's reference and doesn't support such a beautiful syntax to access its onwardly growing contents during the asynchronous writing into it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@TheOneWithTheBraid Can we please have dartdoc comment for the commandStdout that says it's used for the command output in case a command was executed

Copy link
Contributor

Choose a reason for hiding this comment

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

@TheOneWithTheBraid ping

@TheOneWithTheBraid Can we please have dartdoc comment for the commandStdout that says it's used for the command output in case a command was executed

}) {
if (parseCommands) {
return client.parseAndRunCommand(
Expand All @@ -634,6 +635,7 @@ class Room {
txid: txid,
threadRootEventId: threadRootEventId,
threadLastEventId: threadLastEventId,
stdout: commandStdout,
);
}
final event = <String, dynamic>{
Expand Down
Loading
Loading