Thanks for taking the time to contribute.

But I don't really want to add methods to the Event struct. My preference is to keep a minimal API surface with the bitfields exposed for checking, coalescing, etc. This is based on the design document mentioned in the README.

If you wants additional methods like this, it's easy enough to convert to a new type and provide them there.

An article on how to do this (and how bitfields work) would be a better approach IMO. Happy to take contributions to the fsnotify.org blog:
https://github.com/go-fsnotify/fsnotify.org

I feel like this would be much better if implemented on the Op type directly instead of on Event. I would say that it's similar to the os.FileMode type, that's just a bit field int but has methods IsDir and IsRegular to extract info from it without using bitwise arithmetic directly.

As a bonus, if the Event struct is changed to embed Op anonymously these new methods on Op would be promoted to Event without cluttering its' explicit type (or documentation). I don't see any downside to this. Because the current name of the struct field is the same as the type name it should be 100% backwards compatible and references to Event.Op will still work as expected. Like so:

type Event struct {
    Name string // Relative path to the file or directory.
    Op          // File operation that triggered the event.
}

I like this - but why Has? Why not Is?

IsCreate, IsWrite, etc.

it also wouldn't stop switch code either.

On 20 Aug 2015, at 06:53, Joe [email protected] wrote:

I feel like this would be much better if implemented on the Op type directly instead of on Event. I would say that it's similar to the os.FileMode type, that's just a bit field int but has methods IsDir and IsRegular to extract info from it without using bitwise arithmetic directly.

As a bonus, if the Event struct is changed to embed Op anonymously these new methods on Op would be promoted to Event without cluttering its' explicit type (or documentation). I don't see any downside to this. Because the current name of the struct field is the same as the type name it should be 100% backwards compatible and references to Event.Op will still work as expected. Like so:

type Event struct {
Name string // Relative path to the file or directory.
Op // File operation that triggered the event.
}
—
Reply to this email directly or view it on GitHub.

@matryer
Perhaps because what is Write can't be Remove, but what has Write may have Remove too.

But overall, I agree with @nathany, I don't think any more methods should be added.
This library should stay as minimal as possible and only solve the problem of cross-platform portability, if you want fancy helpers, you can just write a wrapper around this library and use it.

Yeah. You'd almost do away with the bitmask consts in favour of the methods.

But, it's pretty nice as it is too.

On 20 Aug 2015, at 12:09, omeid [email protected] wrote:

@matryer
Perhaps because what is Write can't be Remove, but what has Write may have Remove too.

But overall, I agree with @nathany, I don't think any more methods should be added.
This library should stay as minimal as possible and only solve the problem of cross-platform portability, if you want fancy helpers, you can just write a wrapper around this library and use it.

—
Reply to this email directly or view it on GitHub.

IMHO, the problem with adding these methods is that you either have to unexport the Event.Op (breaking change, methods are more restricted than bitmask) or have overlapping API.

Now overlapping APIs doesn't so bad until you consider that there are 8 backends, so where you need 8 tests, with this new methods you need at least 16, where a feature could have been inconsistent or broken in 8 places, now it can be at least 16.

This is unnecessary complexity that can be shifted into it's own package.

It's more an academic discussion about package API design really. I agree that there's no need to change it now.

On 20 Aug 2015, at 12:30, omeid [email protected] wrote:

IMHO, the problem with adding these methods is that you either have to unexport the Event.Op (breaking change, methods are more restricted than bitmask) or have overlapping API.

Now overlapping APIs doesn't so bad until you consider that there are 8 backends, so where you need 8 tests, with this new methods you need at least 16, where a feature could have been inconsistent or broken in 8 places, now it can be at least 16.

This is unnecessary complexity that could be shifted into it's own package.

—
Reply to this email directly or view it on GitHub.

The use of bitmasks and creation of a wrapper could be documented on the blog. Would anyone like to write it up? https://github.com/go-fsnotify/fsnotify.org

I'm closing this in favour of documentation or a blog post. See #107. Pull requests to the website repo are welcome.