Convert OperatingSystem documentation to Markdown - #5710
Conversation
pekkaklarck
left a comment
There was a problem hiding this comment.
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 | |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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
BuiltIna 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 |
There was a problem hiding this comment.
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. | ||
|
|
There was a problem hiding this comment.
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 | ||
| ``` |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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:
robotframework/src/robot/libraries/String.py
Line 236 in 78dd2a9
| 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 |
There was a problem hiding this comment.
👍 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']}} |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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.
What
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
AI assistance
OpenAI Codex assisted with the documentation conversion, tests, and validation. I reviewed the resulting diff and verification evidence.