-
-
Notifications
You must be signed in to change notification settings - Fork 746
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
Traversal-change #2661
base: main
Are you sure you want to change the base?
Traversal-change #2661
Conversation
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.
Beware I haven’t done much Ruby programming in my life, so don’t take my suggestions at face value.
if @app.config[:traversal_use_any_index] | ||
return @store.find_index_resources_by_prefix("").first | ||
else | ||
return @store.find_resource_by_path(@app.config[:index_file]) | ||
end |
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 believe the find_index_resources_by_prefix
function should return the appropriate resources even when :traversal_use_any_index
is configured to false. This would allow merely calling it here rather than testing that configuration option and falling back to some other behavior when that configuration is false.
More importantly, it would prevent calling the find_index_resources_by_prefix
later, only to find out that it doesn’t return index resources.
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.
Trying to avoid possible regressions with large existing sites with many index files. Need to be very defensive when there are decade-old projects still running. Can make this the default in the future if all goes well.
@@ -244,6 +256,20 @@ def ensure_resource_list_updated! | |||
@resources.each do |resource| | |||
@_lookup_by_destination_path[resource.destination_path] = resource | |||
end | |||
|
|||
if @app.config[:traversal_use_any_index] |
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 believe @_lookup_indexes_at_path
should be populated even when this condition evaluates to false.
For now, the find_index_resources_by_prefix
function will not provide the appropriate index resource in case the :traversal_use_any_index
configuration option is false because of this. Obviously that function can work around that dictionary not being populated. However I believe it would be easier (and not much more expensive) to just make sure it is always populated.
In case it is decided to have this dictionary always populated, the test condition line 267 needs to read the :traversal_use_any_index
and the :index_file
.
In case it is decided to have this dictionary only populated when :traversal_use_any_index
is true, I believe this limitation needs to be clearly documented.
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.
Same answer as above, but I do agree around the surprising behavior. I'll mark find_index_resources_by_prefix
as a private method so others don't rely on it.
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.
Fair enough, that makes sense. I believe I would not be as cautious myself, but I definitely understand.
Beware about your future self though: make sure you document this behavior so you’re not surprised when calling find_index_resources_by_prefix
for some new use case a few years from now.
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.
Good idea. I'll have it throw if it wouldn't work
@@ -156,6 +156,18 @@ def find_resource_by_destination_path(request_path) | |||
end | |||
end | |||
|
|||
# Find a resource given its destination path |
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.
This doesn’t seem quite right. I would rather write: “Find index resources given its destination directory path”.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
No description provided.