Path Content

[ghost]

This reads content from

• directory
• file

And returns an array of content. Next step would be to use it "everywhere" in the code. rubcop complains if I use ArgumentError although that should be ok. For now I disabled the check.

[maxmeyer]

Ok. I liked the name read_content read better. At first I used content but that doesn't work because rspec use this as well.

[jarl-dk]

I think reading from a file is a fundamentally different thing that getting a list of files and directories for a given path. I don't like the two responsibilities collapsed in one method. I prefer single responsibility principle and separation of concern.

[maxmeyer]

So you would prefer to have something like read_directory and read_file?

[mattwynne]

I agree with @jarl-dk. How about ls(path) and read(file)?

[maxmeyer]

Great. Good idea. Will fix this.

[mattwynne]

ooh, or even maybe cat(file)?

[maxmeyer]

aaah... we should decide how we name commands... ;-) ls or list. For me read is ok.

[maxmeyer]

... and one thing I forgot... read does not concatenate files which cat does...

[maxmeyer]

So I prefer to have 3 separate commands:

• read - read file content
• list - list directory content
• concatenate - concat file content (using read in a loop)

Since all other commands are written in full, I would prefer list written in full as well.

[mattwynne]

Sure, do it. I just though it would be fun to emulate bash commands.

[jarl-dk]

How about instead of having list for directories, then having a #dir to return a Dir object. Then the user could do dir("%/subdir").each {|entry| handle_it(entry)}.

[maxmeyer]

Sure, do it. I just though it would be fun to emulate bash commands.

@mattwynne My first thought was "cool, I like this", but then "what if we decided to implement it differently then the shell command is implemented". I think, if we decide to do that we need to provide it either as a separate module to include or rename all other methods where a similar bash command exists.

How about instead of having list for directories, then having a #dir to return a Dir object. Then the user could do dir("%/subdir").each {|entry| handle_it(entry)}.

@jarl-dk I like the idea, but would say it's another command and could be used internally as well.

I know, that a custom matcher would be better, but this reads good as well, doesn't it?

it { expect(list('dir.d')). to include 'file.txt' }

[maxmeyer]

BTW:
Is this normal, that I can change YOUR comments?

[mattwynne]

Is this normal, that I can change YOUR comments?

I think it’s a perk of being a project contributor. I sometimes use it to tidy up when people don’t know how to use markdown properly etc.

[ghost]

@mattwynne This one, too. Please.

[mattwynne]

Isn't this comment wrong now?

[maxmeyer]

Sure. Fixed.

[mattwynne]

Why do we need this method? If we have list and expand_path I think people can compose this themselves. Seems like API noise to me.

[maxmeyer]

Oh, I didn't noticed that you mean all_paths. This one was added by me to reduce code duplication in check_file_presence and in Then /^(?:a|the) file matching %r<(.*?)> should (not )?exist$/ do |pattern, expect_match|. And of course users can compose this, but I really like to write in RSpec `expect(all_paths).to include ('dir.d'). It reads well and reduces the boilerplate code needed to set this up. And this one was added ealier 😄.

[mattwynne]

Apart from minor comments this looks great @dg-ratiodata. I really like how you're making the effort to document and spec everything. You're a great asset to the team!

[mattwynne]

Please merge this in when you're ready.

[maxmeyer]

You're a great asset to the team!

Thanks a lot!

I really like how you're making the effort to document and spec everything.

Thanks again. I started to refactor the whole feature suite. Will push this soon.

[maxmeyer]

I leave all_paths for now. Otherwise there would be code duplication which I do not like.