Implements yammer message list#1171
Conversation
|
Thank you! We'll review it shortly 👍 |
VelinGeorgiev
left a comment
VelinGeorgiev
left a comment
There was a problem hiding this comment.
Thank you very much Patrick @plamber ! I tested the command and it worked for many of the scenarios so well done. It looks like a complex one and you've managed to make it work.
There are several suggestions for improvement inline in the code as well as two here:
- The command should show messages when the
--outputis text. I could not see any messages when running it in text mode. See the screen above
Yes, I can see a bunch of ids, but no messages are listed, and the command I believe is about messages :)
. Could you make sure you display in text output one of the message formats we get when output is json
parsed, plain or rich? Probably plain? Maybe you can remove the sender or created date or both to save space for the message.
- When I run the command like this
yammer message list --threaded extended, it still behaves as I would run it likeyammer message list --threaded true. If I completely remove the option likeyammer message listthen it shows more info.
Can I suggest we turn the --threaded option into switch option meaning when it is specified then the threaded=true, when it is not specified then you can automatically assign threaded=extended. This will save you some checks you are doing atm . Please refer to my comments in the code.
|
@VelinGeorgiev I simplified the threaded option. You misinterpreted it. These are two separate use cases provided by the Yammer API (thread first message and thread fist message + first two messages of that thread (extended)). I am going to simplify this API by providing just the threaded option without the extension. I am trying to implement the rest as suggested. I might need help from you on a specific request since I am not 100% sure how to do it properly. |
|
Would it make sense to show the first n characters of the message? Otherwise, it's hard to find out which message is which based just on its id. |
|
@waldekmastykarz what about returning the first 25 chars? |
|
@VelinGeorgiev and @waldekmastykarz I think the revisited version is better now. I am outputting the first 35 chars of a message in the "test" output version and add "..." in case more text is available. I simplified the "threaded" part and kept the "extended" parameter completely out of the equation for now. In case it becomes required in the future, we could consider an extension of this command. |
Thanks for the clarification @plamber, I indeed did not know how that thing is working. It is somewhat odd the way the Yammer guys implemented this. I would suggest keeping just option one as you suggested and if there is a need to pull more messages from the specific thread, then we could use another API to pull just the thread messages. I hope that there is such an API. @waldekmastykarz what do you think? |
|
Makes sense @VelinGeorgiev 👍 |
|
@VelinGeorgiev I just pushed a small version changing just the order of the commands in the docs and adding the type (n: any) |
There was a problem hiding this comment.
Waldek @waldekmastykarz , Patrick @plamber I just noticed that initializing property in the constructor does not work quite as we expect when CLI in interactive mode. Take this example:
I would think that the items array should be [] every time we call the command, but in interactive mode the constructor is being called only once ever so we get more and more items in the array with every new command. Here is an example:
A simple solution to prevent this could be to reset the value of the property under the commandAction method.
This is a valid scenario for all the commands. I would usually use variables inside commandAction method instead of class properties, but I was not doing it intentionally. This is why I caught this behavior just now.
|
@VelinGeorgiev let us go for this change. Do you change it directly? |
VelinGeorgiev
left a comment
VelinGeorgiev
left a comment
There was a problem hiding this comment.
Thank you Patrick @plamber ! Your PR will be merged from here: https://github.com/VelinGeorgiev/office365-cli/tree/yammer-message-list
You can find some minor changes I did in the comments below.
| }; | ||
|
|
||
| public commandAction(cmd: CommandInstance, args: CommandArgs, cb: () => void): void { | ||
| this |
There was a problem hiding this comment.
Added this line of code to reset the items array in CLI interactive mode this.items = []; // this will reset the items array in interactive mode
| } | ||
|
|
||
| if (args.options.threaded) { | ||
| if (endPoint.indexOf("?") > -1) |
There was a problem hiding this comment.
Added curly brackets to improve readiness.
|
Merged manually. Thank you! 👏 |
5 participants
Implements #1104