plugin name is wrapped in double quotes instead of single quotes

[catdad]

I know this is really small and petty, but my tests are failing, so I feel it needs to be mentioned.

I am working to update all my gulp tasks to use the specific modules that were inside gulp-util. As such, I am using this module now to generate task errors. In gulp-util, the module name would be wrapped in single quotes, producing a message like this:

[18:16:17] Error in plugin 'graceful-gulp'
Message:
    pineapples

In this new module, the name is wrapped in double quotes, producing a message like this:

[18:16:17] Error in plugin "graceful-gulp"
Message:
    pineapples

Since I went out of my way to test what my modules log (to make sure it's consistent, and that it indeed happens), my tests are failing. It seems wrong of me to release a major update for such a change, but I also have no good way of knowing if my users depend on this (probably not, but who knows).

Would you entertain the idea of adding a config option for this? Again, I know it's silly and petty to bring this up, but bringing it up seems better than possibly breaking code.

[phated]

You're tests are much to fragile. You should be testing for something like expect(error.message).toContain("graceful-gulp")

That being said, I'll think about changing this.

[phated]

cc @stevelacy @demurgos thoughts?

[stephenlacy]

In general double quotes are preferred for containing free strings due to the ' apostrophe character that may appear in free text.

[yocontra]

I made the original decision to use single quotes a long time ago - IMO I think double quotes are the right move here since other error representations use them. Or no quotes at all, which is how language errors are reported.

It does suck that we broke it and didn't note it, but I don't think it is a huge deal for the few people who tested this way to update their tests. Wouldn't be in favor of a config option, we already have a shitshow of config. Sorry we broke your stuff!

[demurgos]

I agree with the overall sentiment of @contra. The change wasn't really needed but it feels more natural now. The library improved but the change itself broke some things. Sure, the tests were fragile but we could do better by explaining what you can rely on and what not.

Changing back to single quotes may cause the same issues and I don't think there's need to add a config for this.

The main reason why I think that this should not be configurable is because formatting the error message is not the point of this library. When I look at the documentation and tests, there is no mention of the expected toString result, not even a single test. Sure, the error message is nice and has some colors, but in my opinion the point of this library is to improve the semantics of the errors by guaranteeing the existence of the plugin property, exposing the custom properties of the base error and providing access to the source of the error with fileName and lineNumber.

I think that a better solution could be to update the readme to explain why you should use plugin-error: "Error handling for Vinyl plugins." is a bit vague. For example:

• Use this lib so your plugin messages are formatted the same way as the other plugins (consistency)
• Use this plugin so consumers of your plugin can easily catch errors and filter them by plugin name (handle the errors programmatically)
• Both

I'd say that formatting should be handled at a higher level and not be a concern for plugin authors. By the way, expect(error.message).toContain("graceful-gulp") is not going to work. What you see in the first post is the result of .toString. What you should test at the plugin level is expect(error.plugin).toBe("graceful-gulp") or expect(error.message).toBe("pineapples") (this is what plugin-error is tested to do).

Edit: It reminds me of a Chai issue I'm following. They also have some formatting issues and are discussing at which level formatting should be handled. The errors returned by chai (assertion-error) have some special properties and formatting is handled by mocha, but bringing error formatting to a lower level would allow finer control. So it is not always a clear cut and better defining the role of this lib would help by preventing people from testing undocumented behavior.

Edit 2: My current view on the issue is that error.message should be viewed as error.debugMessage: this is a message for developers when things have gone wrong. You just want error.toString it to be complete so you can identify the problem and take actions; you don't really care if the plugin name is quoted or colored: it's nice to have but not essential.

[phated]

Alright, I'm going to close this and create a "Improve documentation/Readme" issue with @demurgos suggestion.

[phated]

Closing in favor of #9

[catdad]

Wow, I missed this whole discussion. Some thoughts, since this went into a bunch of different directions.

My module delivers some functionality. It does so regardless of its dependencies... meaning that a user of my module does not need to understand what my module's dependencies are, what they do, etc. My users only need to understand the functionality that my module delivers. As such, I test the functionality that I deliver. Logging errors is one such function important to my module, and I test exactly what errors it logs. If the error is changed, that's not a matter of discussing the changes in my dependencies... that's a change to my users. As such, even when functionality is provided by a third-party module, like plugin-error, it is still my responsibility to test the behavior of my module. As such, my test are exactly as they should be. They test the message that my users will see when using my module. I could test error.message or error.plugin, but neither of those are what my user sees. My user sees the message printed to the console. I could choose to change the way those errors are generated if I want to, but in order to not require a breaking change, I need to assure that the error my users see is still the same as it was before, and that is exactly what my tests accomplish. That is a long-winded way of saying that plugin authors -- being the ones delivering functionality -- should absolutely be worried about what the errors they are generating, as they are part of that plugin's API and that plugin's functionality.

[phated]

You do realize you replaced a module with a completely different module, right? In most people's book, that's a breaking change...