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

Update windows watcher to send a single rename event with both the old and new path #45

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

iwubcode
Copy link
Contributor

@iwubcode iwubcode commented Jul 4, 2024

Here's the fix for #44 .

A general comment for you to consider in the future, could we avoid heap allocation by using an optional instead? This would also allow us to leverage the copy constructor here.

@e-dant
Copy link
Owner

e-dant commented Jul 5, 2024

could we avoid heap allocation by using an optional instead? This would also allow us to leverage the copy constructor here.

Something like that sounds nice. However, we can't avoid a heap allocation unless we make the event structure non-recursive. (Need some indirection in that case.)

It's recursive because:

Currently, the only possible associated event is a renamed-to event. In the future, we may store other kinds of associated events there. Ownership changes are a good candidate. This kind of structure allows more room for future changes if needed, although admittedly the intended use of associated events may be a bit less obvious to the user than I’d like.

It was meant to give us some room for future changes. There are no filesystem events that require more than a pair of events, so it might not be necessary at all.

We can probably just store a bare event lookalike with just the path/path-type/event-type/event-time in an optional or something similar.

@e-dant
Copy link
Owner

e-dant commented Jul 10, 2024

This can go out in a patch release soon.

Our Conan friends tend to be really on top of updates, so if that's something you use then this should be available there some time shortly after.

@e-dant e-dant merged commit 7b9dfae into e-dant:next Jul 10, 2024
2 of 9 checks passed
@iwubcode iwubcode deleted the windows_rename branch July 10, 2024 14:50
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

2 participants