Add support for table commands with return data - and DownloadSourceCodeCommand. - #439
Add support for table commands with return data - and DownloadSourceCodeCommand.#439Jayson Maxson (jmaxson-ms) wants to merge 15 commits into
Conversation
…ntime to the SDK.
Rename DownloadSourceCodeCommand.
6d3b196 to
7b68d36
Compare
Jayson Maxson (jmaxson-ms)
changed the title
| /// recognize (and can execute) it. Unknown command types may be safely | ||
| /// ignored. | ||
| /// </summary> | ||
| public abstract class TableCommand3 |
There was a problem hiding this comment.
I think we should just change this to an interface. Easier to mock when testing
There was a problem hiding this comment.
Another benefit of making it an interface is you can make implement it directly in the legacy classes like TableCommand2. No need for the standalone adapters
There was a problem hiding this comment.
Jing, I think we should talk offline about this proposed change. I'm not sure that, when thinking about what the proposed change would entail, it would work as smoothly as it seems at an abstract level. (wrt to the second part of your comment specifically)
There was a problem hiding this comment.
We're making this an interface but leaving the adapters.
| /// <see cref="VoidTableCommandResult"/> for commands that do not | ||
| /// produce a meaningful result. | ||
| /// </remarks> | ||
| public interface ITableCommand3Result; |
There was a problem hiding this comment.
Is this necessary? If we don't have any constraint on it, why not just allow all types?
There was a problem hiding this comment.
I believe this is leftover from a legacy implementation. Thanks for pointing it out.
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public override Type ContextType => typeof(TContext); |
There was a problem hiding this comment.
forgot to remove these. thanks
| /// <see cref="VoidTableCommandResult"/> when the command does not | ||
| /// produce a value. | ||
| /// </typeparam> | ||
| public abstract class TableCommand3<TContext, TResult> |
There was a problem hiding this comment.
I think this should be an interface too
| /// A command with the same <see cref="TableCommand3.CommandName"/> | ||
| /// has already been added to this instance (case-insensitive). | ||
| /// </exception> | ||
| ITableBuilder AddTableCommand3(TableCommand3 command); |
There was a problem hiding this comment.
Instead of adding a general TableCommand3, we should restrict it to only the table commands we support i.e.
ITableBuilder AddDownloadSourceCodeCommand(DownloadSourceCodeCommand command);
There was a problem hiding this comment.
I think I prefer the more general approach.
- we don't need to ever worry about a huge number of methods
- this allows for a host and a plugin to have the ability to add extra functionality that doesn't exist in the SDK
There was a problem hiding this comment.
Our primary use case is for WPA though. I don't think we should allow people to add commands that WPA doesn't support. That's just misleading
There was a problem hiding this comment.
IMHO if someone wants to develop a different host than WPA, they should fork this repo. This repo needs to first make sure things work with WPA before worrying about other hosts
There was a problem hiding this comment.
This is a different philosophy than we had when creating this SDK. Happy to chat offline about it.
There was a problem hiding this comment.
We've decided to go ahead and make these specific command methods.
There was a problem hiding this comment.
Note that this does not tie the host to WPA, but it does restrict commands to those supported by the SDK.
| /// <summary> | ||
| /// Gets the identifier of the column that this command targets. | ||
| /// </summary> | ||
| Guid ColumnId { get; } |
There was a problem hiding this comment.
I feel this should be an interface method bool IsColumnSupported(IDataColumn) method for ITableCommand3
There was a problem hiding this comment.
Let's chat offline
There was a problem hiding this comment.
Chatting offline, we decided on bool IsColumnSupported(Guid)
There was a problem hiding this comment.
Note that this ties table commands to one or more columns, but we believe this has been and will continue to be the case. If the need arises, we could re-evaluate in future changes.
| /// runtime type of each command to determine whether they recognize | ||
| /// it. Unknown command types may be safely ignored. | ||
| /// </summary> | ||
| public IReadOnlyList<TableCommand3> Commands { get; } |
There was a problem hiding this comment.
I'd like to define an extension method that makes it easier for WPA to know what it needs to handle:
static void Visit(this ITableCommand3 command,
Action<TableCommand2> visitCommand2,
Action<DownloadSourceCodeCommand> visitDownloadSourceCommand,
...) // more in the future
| /// A command with the same <see cref="TableCommand3.CommandName"/> | ||
| /// has already been added to this instance (case-insensitive). | ||
| /// </exception> | ||
| ITableBuilder AddTableCommand3(TableCommand3 command); |
There was a problem hiding this comment.
It doesn't work to return ITableBuilder here if we want ITableBuidlerWithRowCount to also use this.
2 participants
This branch introduces a new generation of table commands (TableCommand3) that supports async execution, strongly-typed contexts and results, and column-scoped commands. It also adds the first concrete TableCommand3: DownloadSourceCode. Existing ITableCommand / ITableCommand2 continue to work via adapter types now hosted in the SDK.