-
-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Fix false positive conflicts for static files in a collection #9141
Fix false positive conflicts for static files in a collection #9141
Conversation
A couple questions that arose when I was looking into this:
|
Thank you for submitting this proposal @yboulkaid, but I am not liking the double iteration in the new implementation. Can you give an example of the stated false positives? For context, two Ruby strings To maintain plausible backwards-compatibility, we cannot remove existing behavior, only add to it. In #9139, I am suspecting one of the used plugins because the |
I agree that the double iteration isn't very elegant, but it's the only way to remove duplicates after they have been added :/ Do we have any performance benchmarks we rely on to see how much of a hit this can be? I suspect not that much, but I only have managed small Jekyll sites
The test in this PR can reproduce the false positives. The duplicates also have the same # in jekyll/commands/doctor.rb:130
# in destination_map(site)
binding.irb if thing.path.match?(/extensionless_static_file/)
If we decide that static files in collections should be included both in Looking through this again, I realized that we were also actually writing the same file twice even in So as it stands today we can't assume uniqueness when looping in (For context, adding static files the the collection document list has been added in 2014)
True 👍 |
It's not The root cause behind the issue is that As of now, I am not able to come up with a better solution than what you have proposed. Yet, I am not willing to approve your proposal in current state either. |
You're right, thanks for the correction. Now we have an idea about what the issue is at least.
Could you elaborate about what in the proposed solution poses a problem? Is it the concept of the double loop, the performance implications, or something else? I can spend some more time looking into this, and knowing what is wrong with this solution helps giving me a direction |
Primarily the double iteration. Secondarily (to a very less extent because modern Ruby versions are better optimised for handling the scenario) stashing the block as a proc for subsequent use. The ideal solution according to me involves defining new variables and methods to ensure consistency and backwards-compatibility but that could involve making changes to classes other than |
I rewrote the |
Thank you for getting back at tackling this @yboulkaid. I must admit that the current diff looks a lot better than the prior ones. Jekyll uses a similar procedure to traverse through layouts of a rendered resource. However, the attached test needs some cleaning up. You have currently created a new context under previously existing context. That is not correct since we're testing a separate / different Doctor command procedure. Additionally, the expectation is a bit vague: # test/test_site.rb
context "static files in a collection" do
should "not be revisited in `Site#each_site_file`" do
...
end
end |
Thanks for the detailed comments. I agree that the tests were not optimal, they were testing a symptom rather than the root cause (because that's what got me into this rabbit hole 😄) I rewrote the test following your recommendation, I think it became much nicer :) What do you think? |
|
||
visited_files = [] | ||
site.each_site_file { |file| visited_files << file } | ||
assert_equal visited_files.count, visited_files.uniq.count |
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.
I also confirmed that this failed with the old implementation
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.
LGTM! 👍
The diff looks very nice now. 👍 |
Thanks @yboulkaid |
Youssef Boulkaid: Fix false positive conflicts for static files in a collection (#9141) Merge pull request 9141
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.
Forgot to submit my review.
@@ -360,9 +360,13 @@ def documents | |||
end | |||
|
|||
def each_site_file | |||
seen_files = [] |
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.
One optimization is to use a Set here. Faster than array for the call to include?.
…#9141) Merge pull request 9141
This is a 🐛 bug fix.
Summary
This PR fixes some cases of false positive reports of file conflicts
when running
jekyll serve
.Context
In #8961 we added the static files in collections to
Jekyll::Site#static_files
.Static files in collections were part of the
Site#documents
(they getadded here), so when we added them to
static_files
they were nowin two places.
As,
Jekyll::Site#each_site_file
traverses bothstatic_files
and
documents
, those entries get duplicated. This in turn shows up inthe output of
Jekyll::Commands::Doctor
as a false positive:This PR fixes this issue by making the
each_site_file
method go througheach file only once, thus fixing the issue.