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

remove empty unused method #16303

Merged
merged 1 commit into from
Jul 28, 2014
Merged

remove empty unused method #16303

merged 1 commit into from
Jul 28, 2014

Conversation

rajcybage
Copy link

No description provided.

@rajcybage rajcybage changed the title remove empty method remove empty unused method Jul 26, 2014
@@ -10,9 +10,6 @@ def initialize(detailed = false)
@closed = false
end

def each
Copy link
Member

Choose a reason for hiding this comment

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

Actually @matthewd noticed that this method is here so we are compliant with the Rack specification (c.f. here). Even though this line is not reached because of the X-Cascade, could you add a comment atop this method so this won't be remove in the future please ?

Copy link
Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps just add a test assertion that Boomer instance respond_to?(:each) (or that Boomer has instance method :each)?

Copy link
Member

Choose a reason for hiding this comment

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

👍 on adding a test and have the comment be the test name.

@matthewd
Copy link
Member

@senny yeah? I'm reluctant to have a test of a test dummy, as opposed to the thing that's actually under test... 😕

Would it help if the comment was clearer on the why?

# We're obliged to implement this (even though it doesn't actually
# get called here) to properly comply with the Rack SPEC
def each
end

@senny
Copy link
Member

senny commented Jul 27, 2014

@matthewd I certainly did not look close enough 😓 didn't see that the code we are talking about is not production code.

@senny senny merged commit f6a52f1 into rails:master Jul 28, 2014
senny added a commit that referenced this pull request Jul 28, 2014
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.

None yet

5 participants