Adding alphabetical file sorting #472
Conversation
- added configuration parameter 'log_sorting_method' - Method for sorting logs into directories
- added dependence of sorting options on the parameter 'log_sorting_method':
- with the value ModifiedTime - ['Newest first', 'Oldest first']
- with the value Alphabetical - ['From A to Z', 'From Z to A']
- supplemented sorting of files in routes 'log-viewer.folders' and 'log-viewer.files'
- supplemented tests (Failed tests did not pass before the changes)
docker compose run --rm php composer test
Tests: 4 failed, 248 passed (815 assertions)
Duration: 2.78s
Random Order Seed: 1757604409
FAIL Tests\Unit\LogIndex\LogIndexTest
⨯ it compresses chunk if gzip is available 0.01s
⨯ it can save to the cache after building up the index
FAIL Tests\Feature\LogFoldersControllerTest
⨯ it folders are sorted alphabetically descending when configured 0.03s
⨯ it can get the log files 0.02s
|
Hey @arukompas! How are you? I'd love to hear your thoughts on this pull request. If you need any changes, let me know so I can get them in. Thanks! |
arukompas
left a comment
arukompas
left a comment
There was a problem hiding this comment.
Sorry for taking so long! Been sick for a few weeks, needed time to rest :)
PR looks great, thanks for the included tests as well! The only small request is to rename the variables to make it explicitly clear we're sorting files, not logs.
- config('log-viewer.defaults.log_sorting_method') -> config('log-viewer.defaults.file_sorting_method')
- key 'log_sort_by_time' -> 'files_sort_by_time' in IndexController
- LogViewer.log_sort_by_time -> LogViewer.files_sort_by_time in FileList.vue
docker compose run --rm php composer test
Tests: 4 failed, 248 passed (815 assertions)
Duration: 2.78s
Random Order Seed: 1757604409
FAIL Tests\Feature\LogFoldersControllerTest
⨯ it can get the log files
⨯ it folders are sorted alphabetically descending when configured
FAIL Tests\Unit\LogIndex\LogIndexTest
⨯ it can save to the cache after building up the index
⨯ it compresses chunk if gzip is available
…er.defaults.folder_sorting_method') === FolderSortingMethod::ModifiedTime - sort files within folders after sorting folders in FoldersController - writing additional tests docker compose run --rm php composer test Tests: 4 failed, 260 passed (827 assertions) Duration: 1.73s Random Order Seed: 1760137292 FAIL Tests\Feature\LogFoldersControllerTest ⨯ it can get the log files ⨯ it folders are sorted alphabetically descending when configured FAIL Tests\Unit\LogIndex\LogIndexTest ⨯ it compresses chunk if gzip is available ⨯ it can save to the cache after building up the index
|
Hello @arukompas! I've made the variable renames you requested. However, these fixes have made the code a bit messy and redundant: when sorting directories by modified time and files alphabetically, the files end up being sorted by modification time first and then by alphabet. I aimed for minimal intervention in the original code, so I'd like to get your approval before making any further changes. Here's what I'd like to propose: 1Remove the redundant file sorting in FoldersController::index() by replacing:
2 Rename the FolderSortingMethod class to SortingMethod, though this would require changes to the config file. 3 Replace magic strings 'asc' and 'desc' with SortingOrder::Ascending and SortingOrder::Descending in FilesController and FoldersController. 4 Possibly add methods like $folders->sort(SortingMethod $method, SortingOrder $order) to significantly simplify the code and improve readability. 5 I might be mistaken, but I think there might be an error in this line: I suspect it should be $sortingOrder === SortingOrder::Ascending instead of $fileSortingOrder === 'asc'. I'd appreciate your thoughts on these suggestions before I proceed further. Thank you! |
|
@yakoffka thanks for the updated solution! Merged, will tag a release soon 🎉 As for other comments, I'll try to get them actioned myself if I can. I'll let you know soon. |
|
2 participants
#471