Skip to content
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

Optionally group file list by subfolder #86

Closed

Conversation

mortenscheel
Copy link

I have a project which requires 2 globs in order to find all the log files (both directly in the logs folder and in subfolders)

return [
   // ...
   'include_files' => ['*.log', '**/*.log'],
   // ...
];

This makes the file list a bit unintuitive, since the first file isn't necessarily the most recent.
So I've tried to add the ability to group the file list by subfolder.
The root folder is always first, then subfolders are listed in alphabetical order.

It's just a quick proof of concept. Let me know if you want some changes.

By the way, it's necessary to run view:clear to get rid of the old version of file-list.blade.php. I thought the blade compiler would automatically detect that the cached view should be replaced. Is it that just how it is with vendor blade views?

@Advaith3600
Copy link
Contributor

Advaith3600 commented Sep 6, 2022

Couple of issues with including **/*.log

  • It will only go till the second level
  • In windows, the directory separator is \ and not /, so in the UI, it will not be parsed properly (see screenshot)

image

This is how it should be

image

@mortenscheel
Copy link
Author

What would you say to using Symfony's Finder to gather the file paths in stead of glob()?

protected function getFilePaths(): array
{
    $finder = Finder::create()->in(storage_path('logs'))
        ->files()
        ->depth(config('log-viewer.search_depth', 0))
        ->name(config('log-viewer.include_files', []))
        ->notName(config('log-viewer.exclude_files', []))
        ->sortByModifiedTime()
        ->reverseSorting();

    return array_keys(iterator_to_array($finder));
}

Then I could just include *.log and increase the search depth to get all .log files.
Or you could just omit the depth limitation and let it search recursively by default. But that might be considered a breaking change in some use cases.

@mortenscheel
Copy link
Author

Okay, so I misremembered how Finder's depth() method works. The example in my previous comment doesn't actually work with the default value (0).

I've pushed a simpler implementation which allows users to turn recursive search on (off by default) via a new 'include_recursively' config value.

@arukompas
Copy link
Contributor

@mortenscheel hey! Thanks a lot for the PR 😄

Here's my thoughts:

  1. Thanks for the tip on the Symfony's Finder class. I didn't know about it, might be a lot more versatile than glob matching. I'll have a look into it this weekend. If that's already part of the Laravel framework then there should be no extra dependency required and we can use it.

  2. When submitting frontend changes, it's always a good idea to also show some screenshots of what the change looks like :D Otherwise I can't comment on it until I get some free time and take a look at it myself by cloning and setting up your forked repo.

  3. Is the purpose of this PR primarily to order the folders/files better? Because I understand that's an issue at the moment - there's no good order to the file list displayed.
    If that's the case, I can tell you that my open PR on reworking the index to be timestamp-based will solve this issue. It will sort the folders and files based on the latest log entry found in them. This way you will always see the latest logs first, while still keeping the files from the same folder together, so they're not spread across the whole list.

I'd really like to wait with this PR until the timestamps PR gets merged (this weekend or earlier) because it has quite a few changes to the UI as well. It will then also make it a lot easier to bring forth true collapsible folders into the UI.

@Advaith3600
Copy link
Contributor

I have tested this with windows, and it does parse it properly.

Something that I noticed while testing this out was the exclude_files property in the config file. The Symfony finder implementation excludes files by the name and not the entire path. So if something like laravel.log and folder/laravel.log exists and if we just want to hide the root laravel.log, it is not possible. An alternative for this will be to include multiple exclude options, which will allow the users to exclude files by name and path.

In my opinion, this change from glob to symfony/finder can be its own PR.

Now, talking about the original PR changes, the UI could be formatted such that there will be space between the directory separators.

image

@arukompas yes, symfony/finder comes as a dependency to laravel/framework by default and therefore can be safely added as a dependency to this package.

image

@mortenscheel
Copy link
Author

@arukompas sounds great. If you want to make Finder handle the timestamp sorting, you can just do like in my example: ->sortByModifiedTime()->reverseSorting().

Here's the missing screenshot. I've just added a .text-gray-400 header above each subfolder.
image

@mortenscheel
Copy link
Author

@arukompas This PR is really just a proof of concept, and I agree that migrating from glob to Finder requires more consideration and should be a separate PR. You're welcome to close this PR if you wish, and ping me later if you'd like some help.

@arukompas
Copy link
Contributor

Hey @mortenscheel , thanks for great insights!

I do feel like Symfony finder would be a great addition, but it would be a breaking change if we swapped to it completely. Instead, we could use Symfony's finder in addition to glob patterns which users already use in their config/log-viewer.php configuration files. Perhaps that's something for another day :)

Speaking of folders - just tagged v1.3.1 release which contains collapsible folders! Please update and let me know what you think :)

Other than that, the PR gave me some great thoughts and taught me about Symfony finder! :D But I'll be closing this for now as it's not something we should introduce just yet. Perhaps in v2 where we can have breaking changes.

@arukompas arukompas closed this Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants