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

GH-41410: [C++][FS][Azure][Docs] Add AzureFileSystem to Filesystems API reference #41411

Merged
merged 7 commits into from
May 1, 2024

Conversation

amoeba
Copy link
Member

@amoeba amoeba commented Apr 27, 2024

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.

@amoeba amoeba requested review from kou and felipecrv April 27, 2024 05:09
Copy link

⚠️ GitHub issue #41410 has been automatically assigned in GitHub to PR creator.

@amoeba
Copy link
Member Author

amoeba commented Apr 27, 2024

@github-actions crossbow submit preview-docs

@github-actions github-actions bot added the awaiting review Awaiting review label Apr 27, 2024
Copy link

Revision: 0a5cbb3

Submitted crossbow builds: ursacomputing/crossbow @ actions-14581fd73b

Task Status
preview-docs GitHub Actions

@amoeba
Copy link
Member Author

amoeba commented Apr 27, 2024

@github-actions crossbow submit preview-docs

Copy link

Revision: 703137e

Submitted crossbow builds: ursacomputing/crossbow @ actions-a15f1ef9b4

Task Status
preview-docs GitHub Actions

@amoeba
Copy link
Member Author

amoeba commented Apr 27, 2024

@kou
Copy link
Member

kou commented Apr 27, 2024

Thanks!

Could you fix the "Supported formats" part at http://crossbow.voltrondata.com/pr_docs/41411/cpp/api/filesystem.html#_CPPv4N5arrow2fs12AzureOptions7FromUriERK3UriPNSt6stringE ?

  • "i., ii., ..." style and "1., 2., ..." style are mixed (Can we control <ol> style?)
  • Can we remove needless \ from ...\<account>... in ii.?

@@ -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.
Copy link
Contributor

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.

Copy link
Member Author

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 &amp;#8212; instead of &#8212; so the fix here makes Sphinx output --- which works.

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Apr 29, 2024
Before this change, doxygen converts '@\<' to '\&lt;' which doesn't make sense (it removes the @ and leaves a \). If you escape the @, like '\@\<' you get what we want: '@&lt;'.
@amoeba
Copy link
Member Author

amoeba commented Apr 29, 2024

@github-actions crossbow submit preview-docs

Copy link

Revision: 0b027a2

Submitted crossbow builds: ursacomputing/crossbow @ actions-2675c9b1a4

Task Status
preview-docs GitHub Actions

@amoeba
Copy link
Member Author

amoeba commented Apr 29, 2024

Good eye @kou. Re:

"i., ii., ..." style and "1., 2., ..." style are mixed (Can we control <ol> style?)

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.

Can we remove needless \ from ...<account>... in ii.?

Done. I ended up having to escape the @ which is unfortunate but Doxygen is handling it strangely, see my commit message.

Let me know what you think @kou. Happy to keep researching.

@amoeba
Copy link
Member Author

amoeba commented Apr 29, 2024

The relevant portion of the rendered docs looks like this for me now:

image

Copy link
Member

@kou kou left a 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?

@amoeba
Copy link
Member Author

amoeba commented Apr 30, 2024

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.

@kou
Copy link
Member

kou commented May 1, 2024

@github-actions crossbow submit preview-docs

Copy link

github-actions bot commented May 1, 2024

Failed to push updated references, potentially because of credential issues: ['refs/heads/actions-2ff4f49ee9-github-preview-docs', 'refs/tags/actions-2ff4f49ee9-github-preview-docs', 'refs/heads/actions-2ff4f49ee9']
The Archery job run can be found at: https://github.com/apache/arrow/actions/runs/8903489894

@amoeba
Copy link
Member Author

amoeba commented May 1, 2024

I ended up reverting the changes which fixes the output to appease the linter here. This is how things look now:

Screenshot 2024-04-30 at 4 34 25 PM

I'm not sure of how to get Doxygen to produce the output I want here so maybe we just merge this as-is.

@kou
Copy link
Member

kou commented May 1, 2024

OK. Let's merge this.

@kou kou merged commit 22f88fa into apache:main May 1, 2024
34 of 37 checks passed
@kou kou removed the awaiting merge Awaiting merge label May 1, 2024
Copy link

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.

tolleybot pushed a commit to tmct/arrow that referenced this pull request May 2, 2024
…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]>
vibhatha pushed a commit to vibhatha/arrow that referenced this pull request May 25, 2024
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants