Skip to content

Improve documentation for compactMapValues() of Dictionary #18547

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

Conversation

Moximillian
Copy link
Contributor

The recently added compactMapValues -method in stdlib's Dictionary type copies its documentation from mapValues, which is not accurate description of the method.

Using compactMap in SequenceAlgorithms.swift as an example, rewrite the the documentation for compactMapValues and provide an example of the usage (vs. mapValues).

Copy link
Member

@natecook1000 natecook1000 left a comment

Choose a reason for hiding this comment

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

Thanks for noticing this, @Moximillian! I have one change request in the abstract.

/// Returns a new dictionary containing the keys of this dictionary with the
/// values transformed by the given closure.
/// Returns a new dictionary containing only keys that have non-`nil` value
/// as the result from the transform by the given closure.
Copy link
Member

Choose a reason for hiding this comment

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

Could this say "...only the key-value pairs that have non-nil values..."?

@Moximillian Moximillian force-pushed the improve-compactMapValues-documentation branch from 59b2d80 to 612a820 Compare August 7, 2018 19:48
@Moximillian
Copy link
Contributor Author

@natecook1000: abstract updated, and commits squashed

@Moximillian
Copy link
Contributor Author

Moximillian commented Aug 7, 2018

@natecook1000 any guess on what the complexity notation should be for this method? O(m+n) like in the compactMap? mapValues() is also missing complexity notation.

@natecook1000
Copy link
Member

Right — mapValues and compactMapValues should have the same complexity as map and compactMap. Thanks!

@Moximillian Moximillian force-pushed the improve-compactMapValues-documentation branch from 612a820 to 29552cf Compare August 7, 2018 21:29
@Moximillian
Copy link
Contributor Author

@natecook1000 I have added the complexity notations as well.

Copy link
Member

@natecook1000 natecook1000 left a comment

Choose a reason for hiding this comment

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

Looks great!

@natecook1000
Copy link
Member

@swift-ci Please smoke test and merge

/// Returns a new dictionary containing only the key-value pairs that have
/// non-`nil` values as the result from the transform by the given closure.
///
/// Use this method to receive a dictionary of nonoptional values when your
Copy link
Contributor

Choose a reason for hiding this comment

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

@natecook1000 should it be "nonoptional" or "non-optional"? I would vote for the latter, since it's a little easier to parse. Just wondering, no big deal.

Copy link
Member

Choose a reason for hiding this comment

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

"nonoptional" is the one we use elsewhere, currently — updating the other uses over in #18263. Thanks!

@moiseev
Copy link
Contributor

moiseev commented Aug 8, 2018

@swift-ci Please smoke test

@natecook1000 natecook1000 merged commit c66e136 into swiftlang:master Aug 8, 2018
@Moximillian Moximillian deleted the improve-compactMapValues-documentation branch August 8, 2018 06:34
natecook1000 pushed a commit to natecook1000/swift that referenced this pull request Aug 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants