-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
GH-41410: [C++][FS][Azure][Docs] Add AzureFileSystem to Filesystems API reference #41411
Conversation
|
@github-actions crossbow submit preview-docs |
Revision: 0a5cbb3 Submitted crossbow builds: ursacomputing/crossbow @ actions-14581fd73b
|
@github-actions crossbow submit preview-docs |
Revision: 703137e Submitted crossbow builds: ursacomputing/crossbow @ actions-a15f1ef9b4
|
Thanks! Could you fix the "Supported formats" part at http://crossbow.voltrondata.com/pr_docs/41411/cpp/api/filesystem.html#_CPPv4N5arrow2fs12AzureOptions7FromUriERK3UriPNSt6stringE ?
|
@@ -226,7 +226,7 @@ struct ARROW_EXPORT AzureOptions { | |||
/// overwriting. | |||
/// - When you use the ListBlobs operation without specifying a delimiter, the results | |||
/// include both directories and blobs. If you choose to use a delimiter, use only a | |||
/// forward slash (/) -- the only supported delimiter. | |||
/// forward slash (/) \--- the only supported delimiter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the \-
for? I assumed ---
become an em-dash when I wrote this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was to work around what appears might be a breathe issue, see breathe-doc/breathe#963 for what I think is a similar report. Once whatever the issue is is fixed, we could revert this since it's surprising behavior.
Doxygen turns ---
into <mdash/>
and turns \---
into ---
. For <mdash/>
, Sphinx/Breathe generates &#8212;
instead of —
so the fix here makes Sphinx output ---
which works.
Before this change, doxygen converts '@\<' to '\<' which doesn't make sense (it removes the @ and leaves a \). If you escape the @, like '\@\<' you get what we want: '@<'.
@github-actions crossbow submit preview-docs |
Revision: 0b027a2 Submitted crossbow builds: ursacomputing/crossbow @ actions-2675c9b1a4
|
Good eye @kou. Re:
It doesn't look like it. The Doxygen output is what I expect but it looks like Breathe is getting an incorrect nesting level. Their code is pretty easy to follow except for how the nesting level ends up higher than what we expect. Their docs give some examples and, if you look at the final one in https://breathe.readthedocs.io/en/latest/lists.html, it actually looks wrong to me. So maybe this is by design? I don't see any config options we can use to control this. I opted to refer to list items as (1), (2), etc instead of (i), (ii), etc so that the comment is correct on its own and accept that the rendered docs are quite right.
Done. I ended up having to escape the Let me know what you think @kou. Happy to keep researching. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Could you fix lint error?
Pushed an attempt but it looks like we might have to get creative to satisfy the linter. I'll return to this in a few hours. I originally smushed list items into their own line because they weren't rendering but there's probably a way to prevent a source newline from resulting in a break in the HTML. |
@github-actions crossbow submit preview-docs |
|
OK. Let's merge this. |
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 22f88fa. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…tems API reference (apache#41411) ### Rationale for this change See apache#41410. ### What changes are included in this PR? Just changes to filesystem.rst. ### Are these changes tested? Yes, locally. ### Are there any user-facing changes? These are those changes. * GitHub Issue: apache#41410 Authored-by: Bryce Mecum <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
…tems API reference (apache#41411) ### Rationale for this change See apache#41410. ### What changes are included in this PR? Just changes to filesystem.rst. ### Are these changes tested? Yes, locally. ### Are there any user-facing changes? These are those changes. * GitHub Issue: apache#41410 Authored-by: Bryce Mecum <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
Rationale for this change
See #41410.
What changes are included in this PR?
Just changes to filesystem.rst.
Are these changes tested?
Yes, locally.
Are there any user-facing changes?
These are those changes.