-
Notifications
You must be signed in to change notification settings - Fork 247
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
Don't return incorrect files when there's a file whose name matches a dir #526
Don't return incorrect files when there's a file whose name matches a dir #526
Conversation
before { record.add_dir('path/subdir') } | ||
it { should eq('subdir' => {}) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bryanlira you added this test in #460. Do you remember why you expected it to return this, vs an empty hash? The acceptance test you added in that PR is still passing with this branch, so I'm not sure if this unit test was critical for something else that I'm not seeing it.
Nice, @ghiculescu, I certainly like the simplicity from reverting the code. Let's see how @bryanlira feels about that one spec change. |
BTW this appears a few lines above the first changes: subtree = if [nil, '', '.'].include? rel_path.to_s There's no way subtree = if ['', '.'].include?(rel_path.to_s) |
Let's wait another day or so to hear back from @bryanlira. If we don't hear back, I will assume the spec change is good and release this in v3.3.4. |
Thanks! In the meantime, do you want me to squash my commits or are you happy with the PR as it stands? (I know some projects don't like the "merge and squash" button in github (though I'm not sure why) so I figure I may as well get everything else ready for merge.) |
I think this branch is good as is. I know @ioquatix prefers the Rebase-and-Merge options so there are no merge commits... Github is saying this branch is ready to go with that strategy. |
Simpler attempt at #524. Reverts #460.
Even with the changes in #460 reversed, the acceptance tests still pass. Only one unit test fails, and I'm not sure it's asserting the right behavior. With this approach, the new tests in #524 also still pass.
cc @bryanlira @ColinDKelley