Traversal-change #2661
Traversal-change #2661
Conversation
| 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.
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.
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.
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.
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.
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.
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.
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.