search

ansys / scade-actions

Set of ci-cd actions for Ansys SCADE environments
http://actions.scade.docs.pyansys.com/
MIT License
1 stars 0 forks source link

feat: Overall doc review for public release #15

Closed PipKat closed 6 months ago

PipKat commented 6 months ago

@ansys/scade-experts and @Revathyvenugopal162 The inclusion of other files within a RST file is always a bit more difficult for me to review. I had to edit some YML files so that the lines shown in the doc are complete and meaningful sentences. Please review my edits carefully. Also, I feel like the "True" and "False" for "Required" column in the tables would be clearer if they were "Yes" and "No" instead. However, I made no such changes as I thought the values might have to be "True" and "False" for programmatic reasons. Here is an example of the table that I'm referring to:

image

Note that "Type" doesn't have entries in any of these tables. Either this is an error, or the always-empty column should be removed? It would also be nice if the "Description" fields in these tables aligned for both the input and output. Here is an example of a bad alignment:

image

Revathyvenugopal162 commented 6 months ago

Thanks @PipKat for the review.

  1. The value true or false is given because it's structured that way in the YAML file. I'm open to changing it to yes or no if needed. Ultimately, the decision lies with @ansjhenry

  2. If the object has a specific type, it's crucial to specify it, especially with versions where it can be either a string or an integer. Instead of removing it, I propose adding values. However, if @ansjhenry feels it's irrelevant for these scade actions, we can remove it from the documentation. Keeping it blank isn't preferable to removing it entirely.

  3. Thanks for notifying it, i am trying to align it now.

ansjhenry commented 6 months ago

Hi @PipKat , many thanks for the review.

  1. I agree Yes or No would be more accurate. The values true/false (should be lowercase) come from the syntax of yaml files for actions, and the attribute required is typed as Boolean. I prefer to stick to the syntax so that users read something consistent with the documentation of other actions on the Internet, for example.
  2. @Revathyvenugopal162 I have an issue with the attribute type which is not a meta-data defined in the syntax of GitHub actions, that's why I've removed if from the files, compared to ansys actions. Users who customize the actions may make mistake by changing the type from string to integer, for example, and providing a wrong value. I suggest we remove this column and describe the type in the description if there is a risk of ambiguity, either explicitly or with an example of acceptable value. When there is a default value, like ., make the quotes visible such as ".".
  3. I agree, I don't know if it is possible to align these columns. I'm afraid, if we can provide hard-coded widths, the rendering is not good when we resize the windows, or in the pdf document. I've been thinking to another option consisting in putting both inputs and outputs in a single table. I didn't do it since most of actions do not have outputs and I wanted to keep the documentation homogeneous with ansys actions.
Revathyvenugopal162 commented 6 months ago

Thanks for the deep review on this, @PipKat.

Regarding the suggestion for the Yes and No values, there is a solution. The YML need to use true or false as this is imposed by GitHub actions syntax. However, we can use a Sphinx hook to render these values as Yes or False.

Could you jump on this task when possible, @Revathyvenugopal162? Let's also add this on Ansys Actions.

Sure i can add those

Revathyvenugopal162 commented 6 months ago

Hi @PipKat , many thanks for the review.

  1. I agree Yes or No would be more accurate. The values true/false (should be lowercase) come from the syntax of yaml files for actions, and the attribute required is typed as Boolean. I prefer to stick to the syntax so that users read something consistent with the documentation of other actions on the Internet, for example.
  2. @Revathyvenugopal162 I have an issue with the attribute type which is not a meta-data defined in the syntax of GitHub actions, that's why I've removed if from the files, compared to ansys actions. Users who customize the actions may make mistake by changing the type from string to integer, for example, and providing a wrong value. I suggest we remove this column and describe the type in the description if there is a risk of ambiguity, either explicitly or with an example of acceptable value. When there is a default value, like ., make the quotes visible such as ".".
  3. I agree, I don't know if it is possible to align these columns. I'm afraid, if we can provide hard-coded widths, the rendering is not good when we resize the windows, or in the pdf document. I've been thinking to another option consisting in putting both inputs and outputs in a single table. I didn't do it since most of actions do not have outputs and I wanted to keep the documentation homogeneous with ansys actions.

The type removed from fields in https://github.com/ansys/scade-actions/pull/15/commits/73661ba911cef03f5b389ff170802b0d1be5f5a9

PipKat commented 6 months ago

@Revathyvenugopal162 @jorgepiloto I was assuming that one of you would merge this PR when it was ready. I'm now wondering if you are waiting for me to merge it?

jorgepiloto commented 6 months ago

Hi @PipKat, I just asked @ansjhenry. He is fine merging this pull-request and will open one last before releasing the project. Thanks for your deep review on this!