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

New command: docs #3958

Closed
Adam-it opened this issue Oct 29, 2022 · 31 comments
Closed

New command: docs #3958

Adam-it opened this issue Oct 29, 2022 · 31 comments

Comments

@Adam-it
Copy link
Contributor

Adam-it commented Oct 29, 2022

Usage

m365 docs [options]

Description

Opens CLI for Microsoft 365 docs webpage in the default browser

Examples

Opens CLI for Microsoft 365 docs webpage in the default browser

m365 docs
@Adam-it
Copy link
Contributor Author

Adam-it commented Oct 29, 2022

@pnp/cli-for-microsoft-365-maintainers any comments?

@nicodecleyre
Copy link
Contributor

Perhaps even more interesting is to add an optional option for directly opening the documentation of a command.

example m365 docs browse -c "pp card get" opens the docs webpage for pp/card/card-get

@Adam-it
Copy link
Contributor Author

Adam-it commented Oct 30, 2022

Perhaps even more interesting is to add an optional option for directly opening the documentation of a command.

example m365 docs browse -c "pp card get" opens the docs webpage for pp/card/card-get

@nicodecleyre thanks for your suggestion.
Interesting idea 👍. We could do that for sure 🤩
What would be the use case to read the document spec in browser rather then just adding --help and reading it in console ?

@nicodecleyre
Copy link
Contributor

Perhaps even more interesting is to add an optional option for directly opening the documentation of a command.
example m365 docs browse -c "pp card get" opens the docs webpage for pp/card/card-get

@nicodecleyre thanks for your suggestion. Interesting idea 👍. We could do that for sure 🤩 What would be the use case to read the document spec in browser rather then just adding --help and reading it in console ?

in the browser the example outputs can be much clearer then in the console especially for text & csv outputs

@Adam-it
Copy link
Contributor Author

Adam-it commented Oct 30, 2022

For sure they are more readable on a web page than in console 👍, Not feeling it as I was like thinking this command might be used to quickly get to documentation that is not available via console, like how to authenticate with a custom AAD app, or use --query filters etc 🤔
but TBH this command was just some 'random' idea that got into my mind so why not 😉.
I will add this to the spec as well. Thank @nicodecleyre for your awesome input and reviewing the issues 👍 You rock 🤩

@Adam-it
Copy link
Contributor Author

Adam-it commented Nov 3, 2022

@pnp/cli-for-microsoft-365-maintainers should we ship it? any comments?

@waldekmastykarz
Copy link
Member

If you'd be looking for help of a command, wouldn't it be more intuitive to attach it to a command, like m365 status -hw (which is short for -h -w, where w = web)? It seems odd to me to introduce two different ways to access command's help.

Also, are we expecting another verb on docs than browse? If not, could we shorten it to just docs?

@Adam-it
Copy link
Contributor Author

Adam-it commented Nov 3, 2022

@waldekmastykarz good idea to extend the existing commands with an additional option to browse for help 👍 Would you agree if I open it as a separate issue, and we will remove this functionality from here?
as for the browse I also agree. corrected in spec ✅

@waldekmastykarz
Copy link
Member

Would you agree if I open it as a separate issue, and we will remove this functionality from here?

Definitely!

@Jwaegebaert
Copy link
Contributor

If you'd be looking for help of a command, wouldn't it be more intuitive to attach it to a command, like m365 status -hw (which is short for -h -w, where w = web)? It seems odd to me to introduce two different ways to access command's help.

+1 on this suggestion but I'm not quite sure about the option name web. Personally, I would call the option docs. This would be consistent to name of the upcoming command m365 docs but I could be wrong of course 😄

@Jwaegebaert Jwaegebaert changed the title New command: docs browse New command: docs Nov 6, 2022
@garrytrinder
Copy link
Member

If you'd be looking for help of a command, wouldn't it be more intuitive to attach it to a command, like m365 status -hw (which is short for -h -w, where w = web)? It seems odd to me to introduce two different ways to access command's help.

+1, I think attaching it to the command would be the more intuitive way to approach this.

@Adam-it I'm interested, if you are already on the command line you can return the help documentation quite easily using --help, why do you then need to open it in the browser? What does the browser experience offer that the command line doesn't?

@Adam-it
Copy link
Contributor Author

Adam-it commented Nov 6, 2022

@Adam-it I'm interested, if you are already on the command line you can return the help documentation quite easily using --help, why do you then need to open it in the browser? What does the browser experience offer that the command line doesn't?

TBH this wasn't my intention/initial idea. If you would look at the comments above it is actually brought up by Nico. My initial idea was just to add a simple command that when used will open CLI docs web page on the home site. The idea behind it was to easily get to the right source that for example has additional guidance how to use CLI in docker. Getting to command docs was not the initial plan here as I also agree the console help is sufficient 😉.
But since it was mentioned I thought we may add this functionality along the way, at least open an issue for it at will see if it will be taken. I guess it is more user preference if she or he would like to read help in console on from the web site. TBH I would not use it personally as I usually work on a single small laptop screen and context switching is where I waste most of my working time 😅.

@garrytrinder
Copy link
Member

Thanks @Adam-it, I was just curious, sometimes a new feature is not needed if we look at the reason why it was asked for.

Maybe we could take inspiration from the Get-Help command in PowerShell for the command help.

If you use the -Online flag it opens up the response in the browser, we could add an --online option to be used in conjunction with the existing --help option.

@nicodecleyre
Copy link
Contributor

nicodecleyre commented Nov 7, 2022

Thanks @Adam-it, I was just curious, sometimes a new feature is not needed if we look at the reason why it was asked for.

Maybe we could take inspiration from the Get-Help command in PowerShell for the command help.

If you use the -Online flag it opens up the response in the browser, we could add an --online option to be used in conjunction with the existing --help option.

so if i get it right, that would result in an extra option --online besides --help?
ex.: m365 --help --online
ex.: m365 spo file get --help --online

@Adam-it
Copy link
Contributor Author

Adam-it commented Nov 11, 2022

thanks, @waldekmastykarz, @garrytrinder, and @nicodecleyre for the discussion on this command. Opening this up as a simple command just to start the CLI docs web page in the default browser. For the online help for each command, I go with @garrytrinder suggestion and create a separate issue for it 👍

@waldekmastykarz
Copy link
Member

See my comment on #4037 but tl;dr --online is to generic of a name to be used with all commands.

@Adam-it
Copy link
Contributor Author

Adam-it commented Nov 12, 2022

@waldekmastykarz sure will check. but regarding this issue, and its current status, I suppose all looks fine right?

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Nov 13, 2022

If we implement just what's in the spec and leave what's in the thread be for now, it's good to go.

@Saurabh7019
Copy link
Contributor

Hi, Can I pick this up?

@Jwaegebaert
Copy link
Contributor

Awesome, all yours @Saurabh7019!!

@Saurabh7019
Copy link
Contributor

I intended to use the 'open' package to access the CLI documentation URL, but it appears that 'open' does not function correctly on Windows. I have tested it on multiple devices. As a workaround, I planned to use the 'start' command on Windows and 'open' on Mac and other devices. However, even 'start' is not working in the CLI solution. I am currently exploring alternative workarounds and would appreciate any suggestions you may have.

@nicodecleyre
Copy link
Contributor

CLI documentation URL, but it appears that 'open' does not function correctly on Windows. I have tested it on multiple devices. As a workaround, I planned to use the 'start' command on Windows and 'open' on Mac and other devices. However, even 'start' is not working in the CLI solution. I am currently exploring alternative workarounds and would appreciate any suggestions you may have.

Hi @Saurabh7019, while taking a closer look to the issues in the github repo of open, i stumbled upon an (unresolved) issue where they have the same problem. sindresorhus/open#298

... I can't quite tell, but I have a feeling I know these guys from somewhere.. 🤔

All jokes aside, is it an option to use { wait: true } like @MathijsVerbeeck stated in the issue?

@MathijsVerbeeck
Copy link
Contributor

MathijsVerbeeck commented Apr 25, 2023

All jokes aside, is it an option to use { wait: true } like @MathijsVerbeeck stated in the issue?

Even that is not working unfortunately 😕 (Because, when using OS X or Linux devices, it will specifically wait until the browser is closed)

@Saurabh7019
Copy link
Contributor

I should have read this thread sooner! Looks like we are waiting for the 'open' package maintainers to resolve this issue.

@Saurabh7019
Copy link
Contributor

Do you know why 'exec' isn't working as expected in CLI solution?

const { exec } = require('child_process'); exec(start https://pnp.github.io/cli-microsoft365/`);`

It works fine with the same Node version on the same machine in a different project, but not in this CLI project.

@nicodecleyre
Copy link
Contributor

nicodecleyre commented Apr 25, 2023

since we use it here:

child_process.exec(`npm ${args.join(' ')}`, (err: child_process.ExecException | null, stdout: string): void => {

, i would expect it should work. What's the error that you get?

@waldekmastykarz
Copy link
Member

We already have logic to open URLs in the CLI based on the open package. I suggest, that we keep it consistent across the CLI. If it's not working as expected, then I suggest that we fix it centrally so that it works the same way everywhere, rather than introducing a novel way specific for this command.

@MathijsVerbeeck
Copy link
Contributor

The issue is that we still didn't receive feedback from the owner of the open repo. I could create a workaround, specific for Windows users. But I'm not sure what about future changes.

@Saurabh7019
Copy link
Contributor

The approach of using "start" on Windows and "open" on other devices should work as a workaround. If you approve, I will make the same changes to the other commands.

@waldekmastykarz
Copy link
Member

Since CLI is cross-platform, we need to ensure that whatever solution we choose works on all platforms. Also, if we end up using a different approach than the open package, let's centralize this code and use it in all other places where we now use open and which has the same issue as what we've identified here. If we end up replacing open in all instances, let's remove it from the project altogether to avoid carrying unnecessary dependencies.

@Saurabh7019
Copy link
Contributor

It makes sense. I will avoid using platform specific code, and instead use the 'open' package to complete this command.

Saurabh7019 added a commit to Saurabh7019/cli-microsoft365 that referenced this issue Apr 29, 2023
@milanholemans milanholemans linked a pull request May 2, 2023 that will close this issue
Saurabh7019 added a commit to Saurabh7019/cli-microsoft365 that referenced this issue May 10, 2023
Saurabh7019 added a commit to Saurabh7019/cli-microsoft365 that referenced this issue May 10, 2023
Saurabh7019 added a commit to Saurabh7019/cli-microsoft365 that referenced this issue May 13, 2023
Saurabh7019 added a commit to Saurabh7019/cli-microsoft365 that referenced this issue May 13, 2023
author Saurabh <saurabh.k.a.tripathi@avanade.com> 1682755281 +0200
committer Saurabh <saurabh.k.a.tripathi@avanade.com> 1683972581 +0200

New command: docs pnp#3958

adjusted test to re-run the PR checks

removed package json reference

Command updated to eliminate the need for logging in.

define before and after hooks for the test suite

Incorporated pull request review changes

Rolled back changes related to the 'open' package and included changes from the PR review

define before and after hooks for the test suite

Incorporated pull request review changes

Incorporated pull request review changes

Rolled back changes related to the 'open' package and included changes from the PR review

define before and after hooks for the test suite

Incorporated pull request review changes
Saurabh7019 added a commit to Saurabh7019/cli-microsoft365 that referenced this issue May 14, 2023
author Saurabh <saurabh.k.a.tripathi@avanade.com> 1682755281 +0200
committer Saurabh <saurabh.k.a.tripathi@avanade.com> 1683972581 +0200

New command: docs pnp#3958

adjusted test to re-run the PR checks

removed package json reference

Command updated to eliminate the need for logging in.

define before and after hooks for the test suite

Incorporated pull request review changes

Rolled back changes related to the 'open' package and included changes from the PR review

define before and after hooks for the test suite

Incorporated pull request review changes

Incorporated pull request review changes

Rolled back changes related to the 'open' package and included changes from the PR review

define before and after hooks for the test suite

Incorporated pull request review changes
@milanholemans milanholemans added this to the v6.8 milestone May 15, 2023
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.

8 participants