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

Issue #548: fix error when renaming folder #552

Merged

Conversation

ColinDKelley
Copy link
Collaborator

No description provided.

@ColinDKelley ColinDKelley force-pushed the issue-548/fix-error-when-renaming-folder branch from 6bd5f69 to 6f0f0ec Compare December 31, 2021 16:30
@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest moving the assignment into the expression for readability sake.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Which assignment? rel_path_s =?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

values.key?(:mtime) ? values : {}
subtree.each_with_object({}) do |(key, values), result|
# only return data for file entries
if values.respond_to?(:has_key?)
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for this extra check?

Copy link
Collaborator Author

@ColinDKelley ColinDKelley Jan 6, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@ColinDKelley ColinDKelley Jan 6, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing transform_values is more efficient too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

@ioquatix
Copy link
Member

Looking good.

@ColinDKelley ColinDKelley merged commit 8b66b47 into guard:master Jan 13, 2022
@ColinDKelley ColinDKelley deleted the issue-548/fix-error-when-renaming-folder branch January 13, 2022 21:25
@ColinDKelley ColinDKelley linked an issue Jan 13, 2022 that may be closed by this pull request
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.

Error when renaming folder
3 participants