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

Implemented Dir.children #4808

Merged
merged 1 commit into from
Aug 8, 2017
Merged

Implemented Dir.children #4808

merged 1 commit into from
Aug 8, 2017

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Aug 8, 2017

Closes #4807

Copy link
Member

@jhass jhass left a comment

Choose a reason for hiding this comment

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

Slight improvement to the spec is possible, looks good otherwise.

it "lists children" do
filenames = Dir.children(__DIR__)
filenames.includes?(".").should be_false
filenames.includes?("..").should be_false
Copy link
Member

Choose a reason for hiding this comment

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

How about comparisons comparing the output to Dir.entries here? Like checking that the size is the same minus two and that all returned items are also in the other.

Copy link
Contributor

Choose a reason for hiding this comment

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

Example with size - 2 knows about implementation details, and should be not used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jhass done.

@RX14
Copy link
Contributor

RX14 commented Aug 8, 2017

How about a yielding version?

@Sija
Copy link
Contributor Author

Sija commented Aug 8, 2017

@RX14 I wouldn't mind but there's no yielding version of Dir.entries so why here?

@ysbaddaden
Copy link
Contributor

It would be nice to interate without allocating HEAP memory. I guess #entries should allow the same.

@Sija
Copy link
Contributor Author

Sija commented Aug 8, 2017

@ysbaddaden I concur, yet it sounds like a topic for a separate PR.

@RX14
Copy link
Contributor

RX14 commented Aug 8, 2017

@Sija Isn't the yielding version of Dir.entries Dir.foreach? I think thats's pretty confusing. But yes, we can leave clearing up the naming to a different PR.

@RX14 RX14 merged commit 020796f into crystal-lang:master Aug 8, 2017
@RX14 RX14 added this to the Next milestone Aug 8, 2017
@Sija
Copy link
Contributor Author

Sija commented Aug 8, 2017

@RX14 IIRC it comes from Ruby, but yeah, I'd say it is a bit confusing. Ruby has Dir.each_child yielding method to complement Dir.foreach btw.

@RX14
Copy link
Contributor

RX14 commented Aug 8, 2017

I think it's confusing having different names. We should have entries and each_entry and children and each_child.

@bew
Copy link
Contributor

bew commented Aug 8, 2017

@RX14 and what should be Dir.each? should we remove it?

@RX14
Copy link
Contributor

RX14 commented Aug 8, 2017

What's currently Dir.foreach would become Dir.each_entry.

@Sija
Copy link
Contributor Author

Sija commented Aug 8, 2017

@RX14 Why not two versions of Dir.entries and Dir.children with yielding and non-yielding overloads?

@RX14
Copy link
Contributor

RX14 commented Aug 8, 2017

@Sija Because that's not how we do naming of yielding/collection members. Think of String#chars and String#each_char. There are plenty of other examples.

The reason for the existing convention is that the name is more descriptive: foo.each_char do |char| vs foo.chars do |char|.

@Sija
Copy link
Contributor Author

Sija commented Aug 8, 2017

@RX14 Fair point, it is more descriptive indeed.

Val pushed a commit to Val/crystal that referenced this pull request Aug 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants