cli-microsoft365 icon indicating copy to clipboard operation
cli-microsoft365 copied to clipboard

`planner task get` missing validation + docs addition

Open milanholemans opened this issue 3 years ago • 4 comments

Noticed some things about the planner task get command.

  • The command has an alias planner task details get. However this alias is not specified in the docs. https://pnp.github.io/cli-microsoft365/cmd/planner/task/task-get/
  • Noticed a small validation issue. When you provide the option id, you can specify all other options like planId, planName, bucketId, bucketName, ... We should add an extra validation that ensures when the option id is provided, that all other options are not provided.

Examples:

image

image

milanholemans avatar Jun 22 '22 18:06 milanholemans

Hi @milanholemans, the planner task details get command was combined with planner task get a few weeks ago. It will need to be removed entirely in v6.

The other point is a good observation, we'll also need to take that up as a requirement for when picking up the optionSet validation...

martinlingstuyl avatar Jun 22 '22 18:06 martinlingstuyl

Hi @milanholemans, the planner task details get command was combined with planner task get a few weeks ago. It will need to be removed entirely in v6.

I understand that these 2 commands became one. But normally aliases are documented in the docs. Take teams channel member add for example. This command has an alias which is also documented in the docs https://pnp.github.io/cli-microsoft365/cmd/teams/channel/channel-member-add.

So since planner task get has an alias called planner task details get, this should also be documented in the docs which isn't the case right now. https://pnp.github.io/cli-microsoft365/cmd/planner/task/task-get A new section called 'Alias' should be added with the alias of the command.

Or is this not the case here for some reason? 😃

milanholemans avatar Jun 22 '22 21:06 milanholemans

That's true, apparently we missed it during the review. Could you split this issue in two? that way, the docs change can come through faster.

Update: never mind. Let's just keep it as it is. 🤙

martinlingstuyl avatar Jun 23 '22 07:06 martinlingstuyl

My bad, I forgot to write the alias part in the docs. 😅

Noticed a small validation issue. When you provide the option id, you can specify all other options like planId, planName, bucketId, bucketName, ...

For the validation, I also noticed this when I was working on #3344. In this issue I had to combine planId and planTitle with id and title. Which then also needed additional checks within the validate function.

We should add an extra validation that ensures when the option id is provided, that all other options are not provided.

Should we reject the command for users when they also pass along the options, for example, id and bucketId? If they use the Id option, the command then ignores all the others, like in this case, ignores the value of bucketId. Optionally, we can show a warning that it's a redundant option because you are making use of the id.

If it's okay for you guys, then I can pickup this issue.

Jwaegebaert avatar Jun 24 '22 11:06 Jwaegebaert