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

Enhanced RDoc for filter #149

Merged
merged 4 commits into from Jun 28, 2020
Merged

Enhanced RDoc for filter #149

merged 4 commits into from Jun 28, 2020

Conversation

BurdetteLamar
Copy link
Member

No description provided.

lib/csv.rb Outdated
#
# The <tt>:output_row_sep</tt> +option+ defaults to
# <tt>$INPUT_RECORD_SEPARATOR</tt> (<tt>$/</tt>).
# filter(**options) {|row| ... } -> integer
Copy link
Member

Choose a reason for hiding this comment

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

Can we say that "the return value is nothing"?
The current #filter implementation returns an integer but it just so happens that input.each returns integer.
We don't want users to expect #filter returns an integer.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that this is weird. We see this in a number of places. Can we return something useful when possible, or nil otherwise?

Copy link
Member

Choose a reason for hiding this comment

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

I think that it's enough that we say it in document.
Should we return nil explicitly? If we should return something, #filter should return nil.

Copy link
Member Author

Choose a reason for hiding this comment

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

::filter could return the output CSV object or both CSV objects.
::foreach could return the CSV object.
::parse could return the CSV object.

What the user would do with these is none of our business?

Copy link
Member Author

Choose a reason for hiding this comment

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

Or just nil everywhere we're now returning an integer.

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 is no longer a doc-only PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kou, can you take a look?

Copy link
Member

Choose a reason for hiding this comment

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

I don't want to state that returning nil or something is the our spec for now.

In the current our knowledge, we don't have enough reason to state that #filter should return nil, the output CSV object or both CSV objects. We may find enough reason to state that #filter should return XXX in the future. If we state that #filter should return ZZZ now, we need to break backward compatibility.

So I think that we should state that #filter returns nothing or #filter's return value is undefined for now.
What do you think about 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.

Works for me. I'll remove the nil returns and delete the nil returns from the call-seqs.
Some of them say "thingy or nil"; I'll just say "thingy". OK/

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@BurdetteLamar BurdetteLamar requested a review from kou June 27, 2020 23:53
@kou kou merged commit 2c347f9 into ruby:master Jun 28, 2020
@kou
Copy link
Member

kou commented Jun 28, 2020

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants