Skip to content

Convert OperatingSystem documentation to Markdown - #5710

Draft
coyaSONG wants to merge 1 commit into
robotframework:masterfrom
coyaSONG:docs/operating-system-markdown
Draft

Convert OperatingSystem documentation to Markdown#5710
coyaSONG wants to merge 1 commit into
robotframework:masterfrom
coyaSONG:docs/operating-system-markdown

Conversation

@coyaSONG

Copy link
Copy Markdown

What

  • Convert the OperatingSystem library and keyword documentation to Markdown.
  • Add structured Args and Returns documentation for all exposed keyword arguments and typed return values.
  • Convert Robot Framework documentation tables, links, headings, and examples to Markdown equivalents.
  • Add a Libdoc regression test that prevents undocumented arguments and return values.
  • Update the OperatingSystem acceptance-test expectation for the converted deprecation link.

Why

This is the OperatingSystem-only part of #5709. It does not touch Dialogs, Screenshot, String, DateTime, or Collections.

How

The library now declares ROBOT_LIBRARY_DOC_FORMAT = "Markdown". Existing documentation content and behavior are preserved while syntax is converted, and exception behavior remains in the prose where separate Raises entries would be redundant.

Testing

  • invoke format on the three changed files
  • invoke library-docs OperatingSystem
  • Libdoc API unit tests: 6 passed
  • Full Libdoc unit tests: 48 passed
  • OperatingSystem acceptance tests: 279 passed
  • Libdoc acceptance tests: 437 passed
  • Generated JSON: 56 keywords, 0 undocumented arguments, 0 undocumented typed returns
  • AST comparison: implementation unchanged apart from documentation and the documentation-format declaration

AI assistance

OpenAI Codex assisted with the documentation conversion, tests, and validation. I reviewed the resulting diff and verification evidence.

@pekkaklarck pekkaklarck left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks really good in general! There are some recurring issues with examples, Args/Returns placement and linking to other libraries as well as some other smallish issues.

Are you @coyaSONG around so that you can take a look at my comments and fix the issues? If you are busy, I can also merge this PR in its current format and me or someone else can fix the problems.

| `[chars]` | One character in the bracket |
| `[!chars]` | One character not in the bracket |
| `[a-z]` | One character from the range in the bracket |
| `[!a-z]` | One character not from the range in the bracket |

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Headers could be aligned and the extra whitespace from the end removed.

as literal strings, can be escaped with the `Regexp Escape` keyword
from the BuiltIn library.
as literal strings, can be escaped with the
[Regexp Escape](BuiltIn.html#Regexp%20Escape) keyword from the BuiltIn library.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Making references to external libraries actual links is a good idea, but I'm not sure can we count on BuiltIn.html being on the same directory as the docs of this library. When the docs are hosted online that is
the case, but if someone downloads the doc or views this doc via an editor, the link won't work.

Safer alternatives:

  • Link to the online version with a full URL like [Regexp Escape](https:///...#Regexp%20Escape). This has a problem that the URL is pretty long (I needed to cut it in this comment as well) making the source look a bit bad. It may not matter much in general, but if it does, creating a reference target and then using just [Regexp Escape] in the actual documentation would help.
  • Just format the keyword name and make only BuiltIn a link that points to online documentation. Also this requires a longish URL, but this time a reference target for [BuiltIn] could be used also in other places. A drawback is that users would need to find the actual referenced keyword themselves. A direct link would probably be better, even if it would require multiple reference targets.
  • Don't do any linking. We haven't had links earlier so not adding links now is obviously fine as well. I have been thinking that adding links like this would be a good idea, though.

`C:\Users\<user>\robot` on Windows and `/home/<user>/robot` on Unixes.

= pathlib.Path support =
# pathlib.Path support

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The pathlib.Path part could probably be code formatted with backticks. I think the table of contents and everything else will work even if a header has formatting, but that would need to be tested.


Returns:
The command output with a possible trailing newline removed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not entirely sure what is the best place for documenting arguments and return values. I guess it to some extend depends on the keyword, but perhaps it would be best to always have them in the beginning after the "short doc" as in the example below. That's the approach we've used with other libraries.

def example(arg: int) -> int:
    """This is the short doc.

    Args:
        arg: Argument doc.

    Returns:
        Retrun value doc.

    The main keyword doc.
    """

${stdout} = Run /opt/script.sh 2>/tmp/stderr.txt
Should Be Equal ${stdout} TEST PASSED
File Should Be Empty /tmp/stderr.txt
```

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These examples must have the *** Test Cases *** header and a test case name to be syntax highlighted correctly. In most cases using the name of the documented keyword as the test case name is fine. If there are needs for multiple slightly different tests, they should be based based on what they demonstrate.

fails. All values accepted by ``decode`` method in Python are valid, but
`encoding_errors` argument controls what to do if decoding some bytes
fails. All values accepted by `decode` method in Python are valid, but
in practice the following values are most useful:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A link to docs about the accepted error handles could be added. We have just added it to a String library keyword that allow encoded values. See here:

The `errors` argument supports all Python's standard

Should Be Equal ${p2} my/path
Should Be Equal ${p3} my/path/my/file.txt
Should Be Equal ${p4} /path
Should Be Equal ${p5} /my/path2

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 for replacing the old way to show results with Should Be Equal. An alternative would be showing the result with a comment after the keyword call like we did with some BuiltIn keywords, but actually validating the return value makes it easier for us to test these examples.

A problem here is that on Windows you get my\path instead of my/path so the example won't pass. We could only add a note about that, but I think it's better to change the test so that it uses the built-in variable ${/} that is either / or \, depending on the operating system. In practice that would mean using my${/}pathinstead of my/path.

@{p3} = Join Paths my/base example/path/ other one/more
Should Be Equal ${p1} ${{['base/example', 'base/other']}}
Should Be Equal ${p2} ${{['/example', '/my/base/other']}}
Should Be Equal ${p3} ${{['my/base/example/path', 'my/base/other', 'my/base/one/more']}}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There's the same problem with / and \ as with the previous example. In this case using ${/} doesn't work, because it would be replaced with \ on Windows before evaluating the inline Python expression and something like 'base\other' isn't valid. That can be fixed easily by using raw strings like r'base${/}other', but I'm slightly worried this gets rather technical. I don't see any other easy solution so perhaps that's fine.

```

On Windows result would use `\\` instead of `/` and home directory
would be different.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, this keyword already had a note about the ´/and` issue. I'm fine copying the note (minus the home directory part) to the above keywords instead of using ${/}.

if data["returnType"]:
self.assertTrue(
data["returnDoc"],
f"{keyword.name}: return value has no documentation",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Cool that you added a test for this! We probably should make this generic so that it tests all standard libraries. Leaving it like it is now is obviously fine in the context of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants