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

Overall doc review for public release #143

Merged
merged 8 commits into from
Feb 9, 2024
Merged

Overall doc review for public release #143

merged 8 commits into from
Feb 9, 2024

Conversation

PipKat
Copy link
Member

@PipKat PipKat commented Feb 8, 2024

Because "entrypoint" was used almost three times as frequently as "entry point," I changed all occurrences to"entrypoint" descriptive text . Otherwise, I reorganized the README and doc landing page to follow the conventions we've adopted recently. Our overall goal is to provide information (such as installation) in one place only and then reference that place from elsewhere.

The links in the README to the doc sections now point to the dev doc folder so that they work. However, prior to release, they will have to be changed to the stable doc folder.

@RobPasMue I've marked the doc review as completed in Issue #130. You can merge this PR as is or first address my very few comments and then merge.

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Feb 8, 2024
@greschd greschd self-requested a review February 8, 2024 19:19
By default, this file is located in the user config directory (platform-dependent).
Its location can be specified explicitly with the ``ANSYS_LAUNCHER_CONFIG_PATH``
environment variable.
The methods in the ``config`` class manage the default configuration
Copy link
Member Author

Choose a reason for hiding this comment

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

All of the code entities in this paragraph have a noun after them, which is the what the Google developer documentation style guide recommends. However, there are many occurrences of code entities where the noun does not follow. If I felt fairly confident what the noun should be, I added it. Oftentimes, I did not feel confident so did not. This is minor and something that can be done later. I just wanted to point out how we might want to make edits later to more closely follow the styleguide.

@PipKat PipKat requested a review from RobPasMue February 8, 2024 21:19
Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

@greschd - have a look at the PR whenever you have the chance since you know the docs better than anybody. On my side, apart from my comments, all good.

README.rst Outdated Show resolved Hide resolved
doc/source/contribute.rst Outdated Show resolved Hide resolved
doc/source/intro.rst Show resolved Hide resolved
doc/source/user_guide/plugin_creation.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
doc/source/contribute.rst Outdated Show resolved Hide resolved
@@ -1,9 +1,9 @@
.. _cli:

Command line reference
Command-line interface
Copy link
Member

@greschd greschd Feb 9, 2024

Choose a reason for hiding this comment

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

I'm still unsure if the user guide is the right place to put this section. I'd appreciate your input @PipKat:

The content of this section is an auto-generated reference of the command-line options, similar to an API reference (but for the CLI).
Should this:

  • remain here (user guide)
  • get its own top-level section
  • be moved into the "API reference" 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 more explanatory documentation for the CLI is included in the "Getting started", and "Examples" sections.

Copy link
Member Author

Choose a reason for hiding this comment

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

@greschd I think that having it in the "User guide" section is fine. I did wonder if maybe the "Rationale" section should be first though--since it did a good job explaining why I would want to use this utility.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think that makes sense. In general, these docs have two audiences:

  • developers of other PyAnsys packages
  • users of the CLI, which are end-users of the PyAnsys packages

But I think it makes sense to put the rationale page (aimed at other PyAnsys devs) first, simply because the end-users will likely find this through the docs of the library they're directly using first, which can directly link to the right section.

@greschd
Copy link
Member

greschd commented Feb 9, 2024

Thanks a lot for the review @PipKat. It looks good overall from my side; I've just added some minor tweaks.

Apply feedback from dev review

Co-authored-by: Dominik Gresch <greschd@users.noreply.github.com>
Co-authored-by: Roberto Pastor Muela <37798125+RobPasMue@users.noreply.github.com>
@PipKat
Copy link
Member Author

PipKat commented Feb 9, 2024

@RobPasMue @greschd Thanks so much for your thorough review. You caught quite a few errors on my part, which is much appreciated! @greschd I'll let you resolve the last few comments and merge this PR when you are fully satisfied with it!

@greschd greschd enabled auto-merge (squash) February 9, 2024 14:22
@greschd greschd merged commit 022346c into main Feb 9, 2024
25 checks passed
@greschd greschd deleted the doc/overall_review branch February 9, 2024 14:24
@greschd
Copy link
Member

greschd commented Feb 9, 2024

Thanks again for your review @PipKat! Awesome work 🎉

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

Successfully merging this pull request may close these issues.

None yet

3 participants