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

Check all pydoc strings, fix problem with invalid rendered markdown #173

Merged
merged 3 commits into from
Jan 2, 2024

Conversation

GNUSheep
Copy link
Contributor

Fix issue: #169

I think the best option is to use a little bit of markdown, but if u want to use pure pydoc strings, I can easily swap to ">>>" with keeping all the newlines.

Also sorry for not rendering docs, I don't use python often so I dunno how to make module usable in command line, and I don't wanna break your docs while using lazydocs API

Waiting for your respond
GNUSheep

@francium
Copy link
Member

francium commented Dec 27, 2023

Thanks for the PR.

You should checkout the newly added contributing guide in the README. Specifically we'll want to run make docs and ensure the resulting docs/result.md looks correct. You can preview the markdown either on GitHub after committing and pushing. Or you can also setup VS Code and the "Markdown Preview Github Styling" extension and preview within VS Code.

And make sure to commit the updated docs folder after running make docs.

We also need to ensure it renders correctly in editors and IDEs. For this you can also use VS Code with the "Python" extension published by "ms-python". Then just hover over functions and methods and ensure the popup documentation is rendering as expected.

Alternatively, if you use another editor, perhaps Vim/Neovim, you can setup an Python Language Server and verify the docstrings render correctly in the editor that way. There may be a similar thing for Emacs, if you're using Emacs. Pycharm, if you're using that, should support rendering docstrings out of the box without any additional configuration.

@francium
Copy link
Member

francium commented Dec 27, 2023

Also I'm not 100% sure about what syntax to use. The official Python docs are usually written in ReStructuredText I believe. However, LazyDocs may support Markdown docstring syntax.

For ReStructuredText, LazyDocs had a link to this example page for syntax, https://www.sphinx-doc.org/en/master/usage/extensions/example_google.html

We'll also need to ensure common editors like VS Code, PyCharm and Language Server's (Emacs, Neovim/Vim) render the docstrings correctly as well. I have access to all 3 editors, so I can test before merging to verify it's working correctly in all editors.

@GNUSheep
Copy link
Contributor Author

Okay, at first I want to apologize for not reading newly added contributing guide in the README. I did tests, removed all whitespaces, and updated docs.

Also I tested how does it look like in vscode.

While using a little bit of markdown it looks like this:

```python

r: Result[int, str] = get_a_result()
if is_ok(r):
    r   # r is of type Ok[int]
elif is_err(r):
    r   # r is of type Err[str]

```

So the only way to remove that "````python" , it's to use ">>>". It's look way better in vscode, but it's looking kinda weird for lazy docs:

Usage:

r: Result[int, str] = get_a_result()

if is_ok(r):

r # r is of type Ok[int]

elif is_err(r):

r # r is of type Err[str]

@francium
Copy link
Member

Take a look at the failing GitHub actions as well. flake8 is complaining about some code formatting issues. You can also run flake8 locally with make lint

https://github.com/rustedpy/result/actions/runs/7342229052/job/19992515725?pr=173

I'll try to review this PR in detail as soon as I'm able.

@GNUSheep
Copy link
Contributor Author

Take a look at the failing GitHub actions as well. flake8 is complaining about some code formatting issues. You can also run flake8 locally with make lint

https://github.com/rustedpy/result/actions/runs/7342229052/job/19992515725?pr=173

I'll try to review this PR in detail as soon as I'm able.

Done that

src/result/result.py Show resolved Hide resolved
docs/result.md Outdated
for z in get_sync_result_3()
)
final_result: Result[float, int] = await do_async( Ok(len(x) + int(y) + z) for x in await get_async_result_1() for y in await get_async_result_2() for z in get_sync_result_3() )

Copy link
Member

Choose a reason for hiding this comment

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

This rendered example doesn't look right. There's some issue with the docstring.

Tip, you can use this button to preview it in GitHub, in case you had not used it before
image

@francium
Copy link
Member

francium commented Jan 2, 2024

I checked with PyCharm, it doesn't even support non-plaintext documents[1][2]. So we're going to ignore PyCharm and just make sure things render properly in VSCode and other Language Server based editor.

[1]: reStructuredText Python code block in docstring does not render : PY-37743
[2]: Support "code" directive : PY-25658

Copy link
Member

@francium francium left a comment

Choose a reason for hiding this comment

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

Looks great. Thank you for the help @GNUSheep

@francium francium merged commit 262181b into rustedpy:master Jan 2, 2024
5 checks passed
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