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

Docstrings for Timestamp*Array. #988

Merged
merged 2 commits into from
Dec 4, 2021
Merged

Docstrings for Timestamp*Array. #988

merged 2 commits into from
Dec 4, 2021

Conversation

novemberkilo
Copy link
Contributor

Which issue does this PR close?

Re #302

@alamb thought I would add documentation for the Timestamp*Array types. Starting off with TimestampSecondArray to get some quick feedback. A couple of specific questions for you:

  • There are no assertions here ... I am only trying to provide some hints on how to set these up. Is this ok?
  • Curious about how to document things that are currently under the feature flag chrono-tz -- specifically, the ability to specify Australia/Sydney as the timezone. Is this appropriate and if so, shall I just make reference to the flag needing to be enabled?

Thankyou.

@github-actions github-actions bot added the arrow Changes to the arrow crate label Dec 1, 2021
@novemberkilo novemberkilo marked this pull request as draft December 1, 2021 05:27
@novemberkilo novemberkilo changed the title WIP - Docstrings for Timestamp*Array. Docstrings for Timestamp*Array. Dec 1, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #988 (83abb7f) into master (32d8c0a) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #988      +/-   ##
==========================================
- Coverage   82.31%   82.31%   -0.01%     
==========================================
  Files         168      168              
  Lines       48715    48763      +48     
==========================================
+ Hits        40100    40137      +37     
- Misses       8615     8626      +11     
Impacted Files Coverage Δ
arrow/src/datatypes/native.rs 72.91% <0.00%> (-1.56%) ⬇️
arrow/src/datatypes/datatype.rs 65.95% <0.00%> (-0.43%) ⬇️
arrow/src/array/data.rs 79.79% <0.00%> (-0.42%) ⬇️
arrow/src/util/display.rs 20.00% <0.00%> (-0.39%) ⬇️
arrow/src/array/array.rs 85.45% <0.00%> (-0.26%) ⬇️
parquet/src/basic.rs 94.38% <0.00%> (-0.23%) ⬇️
arrow/src/array/equal/mod.rs 93.29% <0.00%> (-0.17%) ⬇️
arrow/src/array/transform/mod.rs 85.10% <0.00%> (-0.10%) ⬇️
arrow/src/alloc/types.rs 0.00% <0.00%> (ø)
parquet/src/file/writer.rs 93.30% <0.00%> (ø)
... and 8 more

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 32d8c0a...83abb7f. Read the comment docs.

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @novemberkilo

There are no assertions here ... I am only trying to provide some hints on how to set these up. Is this ok?

I think it is ok, though adding some assertions as a way to illustrate how to access the values is valuable and common in Rust.

For example, perhaps you could add a call to assert_eq!(arr.value_as_datetime(), "1969-08-25T09:34:49+0:00") to show how it could be accessed, in addition to the comments

Curious about how to document things that are currently under the feature flag chrono-tz -- specifically, the ability to specify Australia/Sydney as the timezone. Is this appropriate and if so, shall I just make reference to the flag needing to be enabled?

I think it is definitely cool to document things under feature flags with a note about what features are needed.

Finally in terms of documenting TimestampNanosecondArray, TimetampMillisecondArray, etc, I recommend leaving a link to the examples on TimestampSecondArray with a note that the others work similarly rather than replicating the examples for each array

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Looks good -- thank you @novemberkilo

@alamb
Copy link
Contributor

alamb commented Dec 3, 2021

@novemberkilo this PR is still marked as a Draft. Do you think it is ready to merge or are there other things you plan to do?

@novemberkilo novemberkilo marked this pull request as ready for review December 3, 2021 22:51
@novemberkilo
Copy link
Contributor Author

Ah sorry about that @alamb - now ready for review and merge. Thanks.

@alamb alamb merged commit 6817c80 into apache:master Dec 4, 2021
@alamb
Copy link
Contributor

alamb commented Dec 4, 2021

Thanks again @novemberkilo

alamb pushed a commit that referenced this pull request Dec 9, 2021
* Docstrings for TimestampSecondArray.

* fixup! Docstrings for TimestampSecondArray.
alamb added a commit that referenced this pull request Dec 9, 2021
* Docstrings for TimestampSecondArray.

* fixup! Docstrings for TimestampSecondArray.

Co-authored-by: Navin <[email protected]>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants