-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
plugin name is wrapped in double quotes instead of single quotes #8
Comments
You're tests are much to fragile. You should be testing for something like That being said, I'll think about changing this. |
cc @stevelacy @demurgos thoughts? |
In general double quotes are preferred for containing free strings due to the |
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! |
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 I think that a better solution could be to update the readme to explain why you should use
I'd say that formatting should be handled at a higher level and not be a concern for plugin authors. 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. Edit 2: My current view on the issue is that |
Alright, I'm going to close this and create a "Improve documentation/Readme" issue with @demurgos suggestion. |
Closing in favor of #9 |
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 |
You do realize you replaced a module with a completely different module, right? In most people's book, that's a breaking change... |
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. Ingulp-util
, the module name would be wrapped in single quotes, producing a message like this:In this new module, the name is wrapped in double quotes, producing a message like this:
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.
The text was updated successfully, but these errors were encountered: