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

Replace globset with glob-match in watcher.rs and command.rs #99

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

abdiu34567
Copy link

@abdiu34567 abdiu34567 commented Oct 3, 2023

Fixes #24

Pull Request: Replace globset with glob-match

Overview:

This pull request proposes replacing the globset library with the glob-match library in the whiz repository.

Changes:

  1. Files Modified:

    • watcher.rs: Implemented glob-match logic, replacing all instances of globset. Ensured that the watcher functionality continues to operate as expected by testing for file changes.
    • command.rs: Removed unused imports related to globset and ensured that the integration with glob-match is seamless. Also made necessary adjustments to data structures (e.g., replacing GlobSet with Vec<String> for pattern storage).
  2. Package Changes:

    • Added: glob-match – An efficient and fast glob matching library.
    • Removed: globset – The previous glob matching library.

Reason for Change:

The glob-match library provides an extremely efficient matching mechanism with zero allocations, no regex compilation, and a linear-time matching process. It's a lightweight and robust alternative to globset.

Testing:

  • All existing unit tests have passed without issues.
  • Manual testing was conducted, especially in terms of file watching, and the results were satisfactory.

Would appreciate a review to ensure the changes are aligned with the repository's standards and objectives. Feedback is welcome.

Copy link
Owner

@zifeo zifeo left a comment

Choose a reason for hiding this comment

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

Nice submission. As the change were rather easy, do you see a way to test this end to end?

@@ -78,8 +79,8 @@ impl Actor for WatcherActor {
#[rtype(result = "()")]
pub struct WatchGlob {
pub command: Addr<CommandActor>,
pub on: GlobSet,
pub off: GlobSet,
pub on: Vec<String>, // Storing raw glob patterns instead of GlobSet
Copy link
Owner

Choose a reason for hiding this comment

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

Will the comment still be useful for developer once the globset is removed? Same applies to a few comments in the PR.

Copy link
Author

Choose a reason for hiding this comment

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

Certainly, an end-to-end test would provide greater confidence that the changes work seamlessly from a user's perspective. Here's a potential approach:

Setup: Prepare a dummy directory structure that mimics a typical use case for the application. This would include various files and directories with patterns we intend to test against.

Execution: Run our application on this directory structure. Use different glob patterns to match files and directories, making sure to test edge cases and any patterns that the old globset handled differently than glob-match.

Validation: Verify that the application's output (or behavior) is as expected. This means the right files are detected, the ignored patterns are truly ignored, etc.

Teardown: Clean up any resources or files created during the test.

I can go ahead and set up such a test. Would this approach meet your expectations? If there's a specific testing tool or framework you'd prefer to be used, please let me know!

Copy link
Owner

Choose a reason for hiding this comment

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

👍 that would work !

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

Successfully merging this pull request may close these issues.

Use glob_match instead of globset
2 participants