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

Use implicit #new in File.open #5337

Merged
merged 2 commits into from
Dec 11, 2017
Merged

Use implicit #new in File.open #5337

merged 2 commits into from
Dec 11, 2017

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Dec 1, 2017

Refs #5335

@straight-shoota
Copy link
Member

This should not close #5335, this fix is just about a side note in that issue.

@Sija
Copy link
Contributor Author

Sija commented Dec 1, 2017

I've changed PR's description, even though mentioned issue is about using wrong ctor (File.new vs new) - which is solved by this PR.

@@ -439,7 +439,7 @@ class File < IO::FileDescriptor
# File.read("bar") # => "foo"
# ```
def self.read(filename, encoding = nil, invalid = nil) : String
File.open(filename, "r") do |file|
open(filename, "r") do |file|
Copy link
Contributor

@bew bew Dec 1, 2017

Choose a reason for hiding this comment

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

For this case and the other below, I'm not sure it's an issue to use File. because the returned object is not a File.

However it's true that if someone wants to override the behavior of open, it is needed to call the implicit open in other class methods.. So maybe it's good, I don't know

Copy link
Member

Choose a reason for hiding this comment

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

The explicit receiver is unnecessary and can be safely omitted.

@larubujo
Copy link
Contributor

larubujo commented Dec 1, 2017

@straight-shoota why not close? theres no bug, only File.new should be new

@straight-shoota
Copy link
Member

straight-shoota commented Dec 1, 2017

Because the issue about File.new vs .new was only the initial incident that made evident a general conceptual issue. This PR will fix the immediately encountered symptom but does not close the discussion about the reasons.

@larubujo
Copy link
Contributor

larubujo commented Dec 1, 2017

@straight-shoota so solution is...?

people make mistakes. theres no way to prevent these errors. sometimes you want File.new sometimes just new. theres nothing to fix. for some it seems everything can be fixed or must be fixed.

@Sija
Copy link
Contributor Author

Sija commented Dec 1, 2017

@straight-shoota I believe there's no conceptual error here, just mistakenly used File.new constructor instead of implicit .new, that's all. Same applies to other places in code where such situation could occur.

@larubujo
Copy link
Contributor

larubujo commented Dec 1, 2017

maybe he means search other places where this happens. then close issue. thats good

@Sija
Copy link
Contributor Author

Sija commented Dec 11, 2017

@RX14 🏓

@jhass jhass merged commit a4ffe5b into crystal-lang:master Dec 11, 2017
@jhass
Copy link
Member

jhass commented Dec 11, 2017

Thanks!

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.

6 participants