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.

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

Planner planTitle inconsistency #3342

Closed
Jwaegebaert opened this issue May 26, 2022 · 10 comments
Closed

Planner planTitle inconsistency #3342

Jwaegebaert opened this issue May 26, 2022 · 10 comments
Assignees
Milestone

Comments

@Jwaegebaert
Copy link
Contributor

Jwaegebaert commented May 26, 2022

Currently when we use a planner title to search for a plan, bucket or task we have 2 different naming conventions. Within planner plan ... we make use of plannerTitle, meanwhile within planner bucket ... and planner task ... we use plannerName.

Here I propose to reform the commands planner bucket ... and planner task ... to the naming convention of plannerTitle. This then aligns most closely with the graph model. plannerPlan resource type

This affects the following command:

  • planner bucket add
  • planner bucket get
  • planner bucket list
  • planner bucket remove
  • planner bucket set
  • planner task add
  • planner task get
  • planner task list
  • planner task set

When we update the option names. Let's also mark the old ones as deprecated, so they can be removed in the next mayor update.

This also affects the following util functions:

  • src/utils/planner.ts - change getPlanByName to getPlanByTitle
@milanholemans
Copy link
Contributor

milanholemans commented May 26, 2022

Maybe we should also rename the planner utility function from getPlanByName to getPlanByTitle. Don't forget to update the function summary as well.

/**
* Get Planner plan by name in a specific group.
* @param name Name of the Planner plan. Case insensitive.
* @param groupId Owner group ID .
*/
async getPlanByName(name: string, groupId: string): Promise<PlannerPlan> {

@appieschot
Copy link
Member

Will be a breaking change and should be matched against next major release. The refactoring of the utility function could be done without impact and could be a separate issue I guess.

@appieschot
Copy link
Member

Just remembered; it would goes in a bit against our https://github.com/pnp/cli-microsoft365.wiki/spec-checklist.md where we state

options follow the naming convention of not repeating the last noun, eg. spo web get --url instead of spo web get --webUrl

@pnp/cli-for-microsoft-365-maintainers please advise ;-)

@milanholemans
Copy link
Contributor

milanholemans commented May 27, 2022

If we use --planTitle for planner bucket ... commands and planner task ... commands, and --title for planner plan ... commands. I don't think there will be any violation against the spec checklist.

@martinlingstuyl
Copy link
Contributor

Agree with @milanholemans: following the naming convention we should not reuse the last noun, but we could change planName to planTitle where used:

m365 planner plan --title sometitle

m365 planner task --planTitle sometitle

Could you adjust the spec @Jwaegebaert?

@milanholemans
Copy link
Contributor

milanholemans commented May 27, 2022

Will be a breaking change and should be matched against next major release. The refactoring of the utility function could be done without impact and could be a separate issue I guess.

Maybe we could create the new options --title and --planTitle and mark --name and --planName as deprecated. Then we could create a new issue to remove the deprecated options in the next major release?

@Jwaegebaert
Copy link
Contributor Author

Agree with @milanholemans: following the naming convention we should not reuse the last noun, but we could change planName to planTitle where used:

m365 planner plan --title sometitle

m365 planner task --planTitle sometitle

Could you adjust the spec @Jwaegebaert?

Here we don't have to update anything special because within the commands planner plan ... the naming convetion is already correct. The commands listed in the specs are the ones that currently make use of planName.

@Jwaegebaert
Copy link
Contributor Author

I just updated the specs to the following suggestions.

Maybe we should also rename the planner utility function from getPlanByName to getPlanByTitle. Don't forget to update the function summary as well.

/**
* Get Planner plan by name in a specific group.
* @param name Name of the Planner plan. Case insensitive.
* @param groupId Owner group ID .
*/
async getPlanByName(name: string, groupId: string): Promise<PlannerPlan> {

Maybe we could create the new options --title and --planTitle and mark --name and --planName as deprecated. Then we could create a new issue to remove the deprecated options in the next major release?

@Jwaegebaert
Copy link
Contributor Author

Jwaegebaert commented May 28, 2022

If all is well, I could take this one.

@waldekmastykarz
Copy link
Member

Like @milanholemans suggested, let's introduce new set of options and in the docs mention that the old options are deprecated. To ensure that we don't lose track of it, let's also create a new issue linked to v6 where we'll remove those deprecated options. Awesome work everyone! 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants