Skip to content

Conversation

@dan-zheng
Copy link
Contributor

Assorted documentation updates.

@dan-zheng dan-zheng added the tensorflow This is for "tensorflow" branch PRs. label Mar 4, 2019
@dan-zheng dan-zheng requested review from mhong and rxwei March 4, 2019 18:52
Copy link

@mhong mhong left a comment

Choose a reason for hiding this comment

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

nice!

///
/// - Parameter numpyArray: The `numpy.ndarray` instance to convert.
/// - Precondition: The `numpy` Python package must be installed.
/// - Precondition: `numpyArray` must be 1-D and have a compatible scalar
Copy link

Choose a reason for hiding this comment

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

do you want to more explicitly describe when a nil value will be returned? Same for other APIs below.

Precondition usually describes cases where the code will lead to undefined behavior (e.g. crash) when the condition is violated.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1. Precondition is definitely not the right term for describing nil. We should look at how common failable initializers are documented in the standard library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for great catch!

A Returns: comment is typically used to describe failable initializers and when they return nil.

Updated in e7920e4, following this stdlib documentation.

- Change precondition comment to return value comment for failable initializers.
- Use `dtype` instead of "datatype".

Address feedback from @mhong and @rxwei.
@dan-zheng
Copy link
Contributor Author

@swift-ci Please test tensorflow

@rxwei rxwei merged commit 575b8cc into swiftlang:tensorflow Mar 5, 2019
rxwei pushed a commit to rxwei/swift that referenced this pull request May 11, 2019
* [Doc] API documentation updates.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tensorflow This is for "tensorflow" branch PRs.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants