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

docs: Update documentation #2832

Merged
merged 1 commit into from
Apr 27, 2022
Merged

docs: Update documentation #2832

merged 1 commit into from
Apr 27, 2022

Conversation

embano1
Copy link
Contributor

@embano1 embano1 commented Apr 26, 2022

Description

  • Remove $ from docs for easy copy and paste
  • Update installation instructions for recent go versions

Closes: #2830
Signed-off-by: Michael Gasch mgasch@vmware.com

Type of change

Please mark options that are relevant:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to
    not work as expected)
  • This change requires a documentation update
  • Build related change

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide
instructions so we can reproduce. If applicable, please also list any relevant
details for your test configuration.

  • Test Description 1
  • Test Description 2

Checklist:

  • My code follows the CONTRIBUTION guidelines of
    this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged

vcsim/README.md Outdated
@@ -78,7 +78,7 @@ flags. Resources can also be created and removed using the API. In fact, vcsim
itself uses the vSphere API generate its inventory.

```console
$ vcsim -h # pruned to model type flags used in the Examples section
vcsim -h # pruned to model type flags used in the Examples section
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nice thing about console + $ command prefix is you can clearly see the commands vs output. Compare the current: https://github.com/vmware/govmomi/tree/master/vcsim#usage
The command and comment are colorized black+grey, output is blue. This is esp. useful when a console section contains multiple commands+outputs.

To w/o the $, all text is blue: https://github.com/vmware/govmomi/blob/c235e4f3282f4f1cee09b7224388b7cd2a30cc9d/vcsim/README.md#usage

I would prefer to keep console+prompt in the sections where include the output. Ok with me to remove the prompt where we don't include any output. But, in those cases you'd probably want to change console to bash, so we get the consistent color (black vs blue).

ex see current black: https://github.com/vmware/govmomi/tree/master/vcsim#install-via-go-get
vs pr blue: https://github.com/vmware/govmomi/blob/c235e4f3282f4f1cee09b7224388b7cd2a30cc9d/vcsim/README.md#install-via-go-install

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, wasn't aware of these little subtleties :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed all non-output lines to bash, reverted back the commands with output to console prefixed by $

Closes: vmware#2830
Signed-off-by: Michael Gasch <mgasch@vmware.com>
Copy link
Member

@dougm dougm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks!

@embano1 embano1 merged commit ac1eba3 into vmware:master Apr 27, 2022
@embano1 embano1 deleted the issue-2830 branch April 27, 2022 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove $ in documentation for easy copy&paste
3 participants