Issue #548: fix error when renaming folder #552
Conversation
ColinDKelley
force-pushed
the
issue-548/fix-error-when-renaming-folder
branch
from
6bd5f69
to
6f0f0ec
Compare
lib/listen/record.rb
Outdated
| @@ -46,16 +46,18 @@ def file_data(rel_path) | |||
| end | |||
|
|
|||
| def dir_entries(rel_path) | |||
| subtree = if ['', '.'].include? rel_path.to_s | |||
| rel_path_s = rel_path.to_s | |||
| subtree = if ['', '.'].include?(rel_path_s) | |||
There was a problem hiding this comment.
I would suggest moving the assignment into the expression for readability sake.
There was a problem hiding this comment.
Which assignment? rel_path_s =?
There was a problem hiding this comment.
I would personally prefer:
if ...
subtree = ...
else
subtree = ...
end
I think it's easier to read and less likely to cause bugs in the future.
There was a problem hiding this comment.
Hmm. To me, that would be a surprising step away from DRY code. I've found the common assignment can be less bug-prone, for example if an elsif is added. And it often identifies a good refactor point where the entire if/else cascade can move into a separate method. I'd rather leave this for now.
lib/listen/record.rb
Outdated
| values.key?(:mtime) ? values : {} | ||
| subtree.each_with_object({}) do |(key, values), result| | ||
| # only return data for file entries | ||
| if values.respond_to?(:has_key?) |
There was a problem hiding this comment.
What's the reason for this extra check?
There was a problem hiding this comment.
The root of this bug is that for folders the subtree has an array (edit: hash) of hashes (that each respond to has_key?). But for files, subtree is a hash with key-values. So if a folder gets renamed into a file, we were crashing here trying to treat a float (mtime) as if it were a hash.
So my hack here is to just treat the symptoms and skip over entries like that, if they happen. I'm sure there's a better answer, and it probably involves the comment higher up about deleting this entire class. But I wasn't able to take on a scope like that.
There was a problem hiding this comment.
Hmm, I think I could address the above more cleanly a few lines up. Maybe check if subtree is an Array at all? I think in the file case it will itself be a Hash.
There was a problem hiding this comment.
Turns out my check for Array wouldn't work, because in fact the top-level object is a hash with the filename as key. The values in turn are hashes with symbols as keys (like :mtime) and scalar values.
There was a problem hiding this comment.
I personally think respond_to? is a hack in this case. Not sure if there is a better alternative without spending some time debugging the data. Maybe you can pp the top level out so we can see exactly what it looks like?
There was a problem hiding this comment.
Agreed--it's definitely a hack!
I certainly could replace respond_to? with is_a?(Hash). Would that seem any less hacky to you? Over the years I've gone back and forth between asking objects is_a? vs respond_to?. The latter is a bit more "duck-typey" but sometimes less clear.
There was a problem hiding this comment.
I'm guessing transform_values is more efficient too.
There was a problem hiding this comment.
Yes, transform_values is probably a bit more efficient. I'd be shocked if there were a case where it mattered here, though. The real cost of this gem is the multiple threads it creates and all the event reprocessing it does.
|
Looking good. |
No description provided.