D338: Doc/overall review_part2

[PipKat]

@hiro727 I didn't realize that you had merged PR #337. I still have to review the docstrings in many more PY files. To push my changes to a new PR, I had to resolve conflicts with what existed in the main branch. I need you to explain to me your template validation check so that any future PRs that I create pass this check. I intend to keep pushing changes to this branch knowing that this check will continue to fail. When I finish with my first-round pass, I will let you know so that you can do whatever is necessary to get the template validation issue fixed. Let me know if you'd like to do things differently than I propose. Thank you!

[drewm102]

@hiro727 I didn't realize that you had merged PR #337. I still have to review the docstrings in many more PY files. To push my changes to a new PR, I had to resolve conflicts with what existed in the main branch. I need you to explain to me your template validation check so that any future PRs that I create pass this check. I intend to keep pushing changes to this branch knowing that this check will continue to fail. When I finish with my first-round pass, I will let you know so that you can do whatever is necessary to get the template validation issue fixed. Let me know if you'd like to do things differently than I propose. Thank you!

@PipKat our PR title template check looks for a string following the regex pattern ((T|S|D|B)[0-9]+\:\s[A-Z]). Basically, it wants there to be some character denoting the work item type (T for task, S for user story, D for defect/issue, etc.) followed by the work item number, followed by a ":" and then a brief string describing the PR. I went ahead and changed the PR title so it passes the check now. Let me know if you have any other questions!

@hiro727 Thanks! Would it be safe for me to assume that if future doc review PRs are necessary, I use the D338 prefix? Doc review might not require another PR--but it's possible. Appreciate your help! #338

@PipKat Yes, that's fine!

[PipKat]

@hiro727 and @SMoraisAnsys Here is another concern I have. Formatting for attributes (properties) look different based on how they are written in the PY files. You can see how these differences are awkward in the following summary table. I think we should always use the same styling, but I don't know which you prefer. Initiatives early on were to make it easily readable, so I would prefer not showing the data type as part of the brief description, but I'm not the decision maker!

@PipKat I think the inconsistencies you are seeing are due to us forgetting to update the document strings for older files as we changed our documentation style during our development process. We adopted the documentation format of : for properties midway through the development process. I believe that most document strings for properties follow this format, but some files that were written prior to this decision might have flown under the radar. If you are ok with it, we would like to keep the format : and revise any files that aren't formatted correctly. If this doesn't meet PyAnsys standard though, we can certainly simplify the doc strings as you suggested. Let me know what you think. Thanks! @bwnedrud @hiro727

@hiro727 I can make the change to showing the data type. Thanks for the feedback.

[PipKat]

@hiro727 One more question, as I go back through and try to standardize property descriptions to indicate the type, does whatever extension you are using support defining the default like this: is_ref : bool, default: False? If so, I can make this change too.

[drewm102]

@hiro727 One more question, as I go back through and try to standardize property descriptions to indicate the type, does whatever extension you are using support defining the default like this: is_ref : bool, default: False? If so, I can make this change too.

@PipKat That's ok, we can just stick with the existing format : . Thanks for offering though!

[PipKat]

@hiro727 One more question, as I go back through and try to standardize property descriptions to indicate the type, does whatever extension you are using support defining the default like this: is_ref : bool, default: False? If so, I can make this change too.

@PipKat That's ok, we can just stick with the existing format : . Thanks for offering though!

@hiro727 So I don't think my question was clear. I was referring to whether we use this for a parameter description:

use_open_region: bool, optional Whether an open region is used. The default is True.

Or, we switch to this format:

use_open_region: bool, default: True Whether an open region is used.

A lot of the newer libraries are using this second method, which does save on wording. However, I don't know if formatting it like this requires a third-party extension.

[drewm102]

@hiro727 One more question, as I go back through and try to standardize property descriptions to indicate the type, does whatever extension you are using support defining the default like this: is_ref : bool, default: False? If so, I can make this change too.

@PipKat That's ok, we can just stick with the existing format : . Thanks for offering though!

@hiro727 So I don't think my question was clear. I was referring to whether we use this for a parameter description:

use_open_region: bool, optional Whether an open region is used. The default is True.

Or, we switch to this format:

use_open_region: bool, default: True Whether an open region is used.

A lot of the newer libraries are using this second method, which does save on wording. However, I don't know if formatting it like this requires a third-party extension.

@hiro727 One more question, as I go back through and try to standardize property descriptions to indicate the type, does whatever extension you are using support defining the default like this: is_ref : bool, default: False? If so, I can make this change too.

@PipKat That's ok, we can just stick with the existing format : . Thanks for offering though!

@hiro727 So I don't think my question was clear. I was referring to whether we use this for a parameter description:

use_open_region: bool, optional Whether an open region is used. The default is True.

Or, we switch to this format:

use_open_region: bool, default: True Whether an open region is used.

A lot of the newer libraries are using this second method, which does save on wording. However, I don't know if formatting it like this requires a third-party extension.

@PipKat Sorry, I misunderstood your previous comment. I tried using the parameter format you described and the documentation generation seems to work fine with it. The resulting documentation for the parameter is as follows:

As long as this looks good to you, let's go ahead and use this new format as you suggested. Thanks!