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

Improve JSON reader documentation #1559

Merged
merged 2 commits into from
Apr 13, 2022
Merged

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Apr 13, 2022

Which issue does this PR close?

re #1538

Rationale for this change

I found @tustvold 's comment helpful #1539 (review) and it wasn't 100% clear to me previously why there was a split between Decoder and Reader. While it was fresh on my mind, I tried to update the doc strings to encode this knowledge for the future

What changes are included in this PR?

Improve doc comments

Are there any user-facing changes?

Better (hopefully) doc strings

@alamb alamb added the documentation Improvements or additions to documentation label Apr 13, 2022
Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

I think you need [``] for the doc link to work correctly

arrow/src/json/reader.rs Outdated Show resolved Hide resolved
arrow/src/json/reader.rs Outdated Show resolved Hide resolved
arrow/src/json/reader.rs Outdated Show resolved Hide resolved
@github-actions github-actions bot added the arrow Changes to the arrow crate label Apr 13, 2022
Co-authored-by: Raphael Taylor-Davies <[email protected]>
@alamb
Copy link
Contributor Author

alamb commented Apr 13, 2022

I think you need [``] for the doc link to work correctly

They worked fine for me locally without ``:
Screen Shot 2022-04-13 at 10 12 38 AM

Putting them in back ticks does make them look nicer

Screen Shot 2022-04-13 at 10 13 02 AM

@tustvold
Copy link
Contributor

TIL, I had always assumed you needed both as that is all I'd ever seen 😅

@codecov-commenter
Copy link

Codecov Report

Merging #1559 (9b43297) into master (228ee36) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1559      +/-   ##
==========================================
- Coverage   82.84%   82.84%   -0.01%     
==========================================
  Files         190      190              
  Lines       54985    54985              
==========================================
- Hits        45551    45550       -1     
- Misses       9434     9435       +1     
Impacted Files Coverage Δ
arrow/src/json/reader.rs 84.48% <ø> (ø)
parquet_derive/src/parquet_field.rs 65.98% <0.00%> (-0.23%) ⬇️
arrow/src/array/transform/mod.rs 86.46% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 228ee36...9b43297. Read the comment docs.

@alamb alamb merged commit d20ebc0 into apache:master Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants