Skip to content

FEAT: Ignore files and folders with regex option. #1015

Closed
distante wants to merge 7 commits intopulsejet:masterfrom
distante:master
Closed

FEAT: Ignore files and folders with regex option. #1015
distante wants to merge 7 commits intopulsejet:masterfrom
distante:master

Conversation

@distante
Copy link
Contributor

@distante distante commented Jan 23, 2024

This PR introduces a control to ignore files and folders using a custom regex when enabled.


I also included a Visual Studio Code Dev Container for people who want to contribute but do not have the environment for it (like me).

The container will install the node modules the first time it is created, and start watch mode on start.

Also, a Nextcloud server with the Memories App as a Custom App is being started with the container, the config and data folders are exposed inside .devcontainer/config and .devcontainer/data but ignored from git.

I haven't done any coding with PHP in a really long time so maybe some tools still need to be added. But I use it to do and test this PR.

image

On the topic of tests. I have not found how the occ scripts are tested. I tried the playwright test but they do not have anything with indexing (also, it needs more configuration for the dev container, for now, the port has to be manually changed). @pulsejet if tests are needed please point me in the correct direction. Thanks!

@distante distante changed the title FEAT: Ignore hidden files and speciak '@' files FEAT: Ignore hidden files and speciak '@' files options Jan 23, 2024
@distante distante changed the title FEAT: Ignore hidden files and speciak '@' files options FEAT: Ignore hidden files and special '@' files options Jan 23, 2024
@pulsejet
Copy link
Owner

pulsejet commented Mar 4, 2024

Thanks for your contribution!

There are a couple of issues here:

  1. Indexing files in hidden folders is intentional, this is required for archive functionality to work. These are actually hidden from the timeline by default (there might be a bug in hidden files)
  2. Not indexing any file in the path that is supported causes issues currently, since the next cron job also attempts to index them. So this keeps making the cron jobs slower and slower. This will be fixed once there's an exclusion / failure list
  3. Better to have a configurable regex pattern for ignore

@pulsejet
Copy link
Owner

pulsejet commented Mar 4, 2024

Hidden files are addressed in 629f40d

@distante
Copy link
Contributor Author

distante commented Mar 5, 2024

So I have to also add that check (now regex) to the cronjob, correct?

Can you point me to where is it?

Thank you!

@pulsejet
Copy link
Owner

pulsejet commented Mar 5, 2024

So I have to also add that check (now regex) to the cronjob, correct?

Actually I forgot we combined both to the service, so that part is fine. In that case the two issues would be:

1/ You're filtering by file basename only, but folders get separately filtered below. So this will actually not ignore the @Recycle folder, for instance.

2/ The first point in my comment. Ignoring folders with a . at the start would break archive functionality. Also files starting with . are already hidden now with the commit referenced above.

My suggestion: use array_filter on $nodes instead of $files to match with the regex pattern. The pattern can/should also be a lot more explicit to what we're trying to do, e.g. default to ^@Recycle$ to avoid unintended consequences (even though it seems unlikely, someone might have photos starting with @ for all we know.

@distante
Copy link
Contributor Author

distante commented Mar 23, 2024

@pulsejet I had added some updates, I have added a toggle to enable the pattern if wanted and a link for some checkers.

I have also added some logs so errors on wrong patterns are shown and logs for folders/files ignored.

I hope it is now Ok.

Pd: I still have to configure the container to resolve all nextcloud/symphony types but it doesn't break anything that isn't already so in master.

@distante distante changed the title FEAT: Ignore hidden files and special '@' files options FEAT: Ignore files and folders with regex option. Mar 24, 2024
@pulsejet pulsejet added this to the 7.2 milestone Apr 3, 2024
@pulsejet pulsejet added the feature New feature or request label Apr 3, 2024
@pulsejet pulsejet modified the milestones: 7.2, 7.3 Apr 6, 2024
Copy link
Owner

@pulsejet pulsejet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm gonna cherry pick the two changes separately, probably make some minor changes to the config. We likely don't need two separate config options, just a single one.

# - apps:/var/www/html/custom_apps
- ./custom_apps:/var/www/html/custom_apps
- ../:/var/www/html/custom_apps/memories
- ./../:/var/www/html/custom_apps/memories
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the ./ at the start required? Seems a bit odd to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm, I think vscode formatter add them. I do not remember that I add it. But I do not remember now to be honest.

dockerfile: Dockerfile
volumes:
# - nextcloud:/var/www/html
# - apps:/var/www/html/custom_apps
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will delete them.

@pulsejet
Copy link
Owner

pulsejet commented Apr 7, 2024

I've updated the dev container setup to start and set up nextcloud with mariadb. Codespaces indicates that everything should be working though I've not tried it locally.

@distante
Copy link
Contributor Author

distante commented Apr 7, 2024

So I should rebase, correct?

@pulsejet pulsejet closed this in a4ae4a5 Apr 24, 2024
@pulsejet
Copy link
Owner

Moved around some things, but essentially the same. Thank you!

image

@distante
Copy link
Contributor Author

Thank you @pulsejet! The last weeks I had almost no time between the work and children to do any changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants