search

neilenns / node-deepstackai-trigger

Detects motion using Deepstack AI and calls registered triggers based on trigger rules.
MIT License
165 stars 28 forks source link

Issue412 #413

Closed salcio closed 3 years ago

salcio commented 3 years ago

Fixes #412

Description of changes

Modified verifyTriggerWatchLocations to use glob.sync(trigger.watchPattern) instead of fs.readdirsync()

Checklist

If your change touches anything under src or the README.md file these items must be done:

salcio commented 3 years ago

Thanks for the PR! A couple of general comments:

  1. There's a build failure that needs to be resolved
  2. Does this work with and without a glob pattern in watchFolder? I assume yes since you only changed dog, and not cat, detector in the sample.json file.

yes it works with and without the glob in pattern.

I'm working on the build failure (something to do with package-lock).

Edit:

Just passed build.

neilenns commented 3 years ago

Great thanks! I'll take another quick look at this tonight then merge it and produce a dev docker image for testing. Will run it locally a bit and ask you to try the dev build as well. If that all works out then I'll publish it as a release.

salcio commented 3 years ago

Thanks for fixing the build failure! Looks like there are two build warnings now that need cleaning up in triggermanager.ts due to imports that are no longer used.

Yeah you are right. I haven't seen the warnings. Now fixed. You might want to consider settings lint to cause build failure when warnings like that happen - will save time :).

neilenns commented 3 years ago

Thanks for fixing the build failure! Looks like there are two build warnings now that need cleaning up in triggermanager.ts due to imports that are no longer used.

Yeah you are right. I haven't seen the warnings. Now fixed. You might want to consider settings lint to cause build failure when warnings like that happen - will save time :).

Solid suggestion. Feel free to open a bug and a PR to get that change in 😂 At the very least open an issue to track it so I can get to it next time I'm touching code.

salcio commented 3 years ago

Thanks for fixing the build failure! Looks like there are two build warnings now that need cleaning up in triggermanager.ts due to imports that are no longer used.

Yeah you are right. I haven't seen the warnings. Now fixed. You might want to consider settings lint to cause build failure when warnings like that happen - will save time :).

Solid suggestion. Feel free to open a bug and a PR to get that change in 😂 At the very least open an issue to track it so I can get to it next time I'm touching code.

Make sense. Issue created #414

salcio commented 3 years ago

Code looks good. I'll re-work the text in the release notes when I push the release out.

One requested change: instead of aiinput/asdf as the sub-folder please change to a real folder name that matches what's inside. I suggest aiinput/dogcamera or similar.

yeah I should have pay more attention before submitting PR. Done some testing and then you see :). It's changed now.

salcio commented 3 years ago

@salcio Heads up it looks like you merged another branch into this one.

Note that if you are planning to open a future pull request to archive the input files I'm unlikely to take it. I don't believe this system should be responsible for managing files it didn't create.

uuu I didn't want to do that. I wanted to merge the other way. I'll revert that merge.

Makes sense with not touching the files that were created by someone else. I didn't plan to create PR for this as it was more for my setup (ease of adding functionality to existing app rather then creating completely new one)