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

Minor docs fixes in displayio.Bitmap #4651

Merged
merged 3 commits into from
Apr 29, 2021
Merged

Minor docs fixes in displayio.Bitmap #4651

merged 3 commits into from
Apr 29, 2021

Conversation

lesamouraipourpre
Copy link

No description provided.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Sphinx syntax is not obvious; it's not markdown, though sometimes it looks like that. You'll want to try to build to docs locally to make sure any changes will work, such as the new cross-reference you propose.

shared-bindings/displayio/Bitmap.c Outdated Show resolved Hide resolved
shared-bindings/displayio/Bitmap.c Outdated Show resolved Hide resolved
shared-bindings/displayio/Bitmap.c Outdated Show resolved Hide resolved
@lesamouraipourpre
Copy link
Author

I agree that Sphinx is not obvious in what is the best way to highlight things as code and cross-reference to other documentation.

I had tested a local docs build and the changes made, the single quotes, get rid of the red links (generally interpreted as errors or missing links) that are currently on the website . I can rechange these back if you prefer but I think that __getitem__ and __setitem__ look better this way. The ulab.numpy.frombuffer would be better as a cross-referenced link but I don't think there is a valid destination in the current docs.

Screenshot 2021-04-23 133012
Screenshot 2021-04-23 133049

@dhalbert
Copy link
Collaborator

dhalbert commented Apr 23, 2021

With your changes, these new warnings below are generated during make html:

home/halbert/repos/lesamouraipourpre/circuitpython/shared-bindings/displayio/index.rst:36: WARNING: 'any' reference target not found: ulab.numpy.frombuffer
/home/halbert/repos/lesamouraipourpre/circuitpython/shared-bindings/displayio/index.rst:66: WARNING: 'any' reference target not found: y * width + x
/home/halbert/repos/lesamouraipourpre/circuitpython/shared-bindings/displayio/index.rst:76: WARNING: 'any' reference target not found: y * width + x

We try to avoid warnings. If we changed double-backtick to single-backtick everywhere, we'd have hundreds of these.

Sphinx is trying to do a cross reference from the text in single backquotes. The RST doc explains the difference:
image
I agree that styling

``inline-literal``

in red may not be the best choice, but that's just the CSS style. We use double-backquotes all over the place for <code>-style markup.

@lesamouraipourpre
Copy link
Author

Weird. I've pushed those changes but I'm not sure if they've pulled over to the PR.
lesamouraipourpre@0be610f

@dhalbert
Copy link
Collaborator

Yes, there's something weird with GitHub. Yours was your first PR against circuitpython, so I had to approve running the Actions, which I did bother to do yet, since I had proposed changes anyway. But then your second push did run the actions, but on only the first commit?? I think GitHub's new Actions permission thing may be buggy.

@dhalbert
Copy link
Collaborator

Try committing and pushing again; I think you can do git commit --allow-empty.

Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

Thanks!

@jepler jepler dismissed dhalbert’s stale review April 29, 2021 13:12

the only edit left is a typo fix

@jepler jepler merged commit 5258969 into adafruit:main Apr 29, 2021
@lesamouraipourpre lesamouraipourpre deleted the minor-docs-fixes branch May 23, 2021 21:29
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.

None yet

3 participants