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

consistently discard return value of constructors #1382

Closed
wants to merge 1 commit into from

Conversation

ynniv
Copy link

@ynniv ynniv commented Jun 6, 2012

As per #1269, the return value of constructors should be discarded.

@ryanflorence
Copy link

Inverse pull request at #1435

@jashkenas
Copy link
Owner

I'm not sure about the semantics of the code in this pull request. I'd be glad to take a closer look at it with an accompanying test that demonstrates the desired change in behavior.

@jashkenas jashkenas closed this Dec 7, 2012
@ynniv
Copy link
Author

ynniv commented Dec 7, 2012

You're just closing out old pull requests, and after much discussion regarding the return value of constructors, I just don't care anymore. If you truly believe that people should not return values from constructors, you should apply this patch to prevent people from returning values from a "constructor" attribute, which is currently capable of changing the value of the object being constructed.

@jashkenas
Copy link
Owner

I truly believe that folks shouldn't return "other" values from constructors, but I also believe that we shouldn't be inserting an extra function wrapper around your constructor just to force the issue. This isn't "nanny-state" programming ;)

@ynniv
Copy link
Author

ynniv commented Dec 7, 2012

Look, I spent considerable time lobbying for ynniv@4d03d09, and wherever that change has arisen (#1269 #1216 and #1094) you have told people that they should not return values from constructors. So either you believe that the framework should explicitly prevent people from returning constructor values or not, but please be consistent.

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

3 participants