-
-
Notifications
You must be signed in to change notification settings - Fork 693
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
[WIP] Message structure refactoring #393
Conversation
you can run |
Some excellent work @svallory. Really good. Quite a lot to digest though - so I'll make some general notes about what I see and we can discuss them further: SyntaxI want to try and avoid too much bikeshedding about syntax but to me it feels like the biggest benefit of the new
The way I see it - all of these could be normalised by using ternary operations based on
Minor Issues
Plus points
Feel free to challenge/disagree with any of my points. These is mostly me thinking out loud while looking through the code - and so every point could be discussed further. |
In addition to @keithamus comments, the new constructors that have been added should have each method commented according to the our style guidelines. It just makes things a lot easier to maintain, especially with multi-author projects like this. While this should not be a high priority and is probably best done close to last, it is a requirement for merging. Overall, this is exciting stuff! Looking forward to seeing how this goes. |
A lot of valid points @keithamus! Before we discuss of them know that the code isn't uniforme yet because it's a WIP, hence the double messages vs SyntaxHere's all you need to know about tokens:
That's all there's to know and I don't think it's hard. Of course, it's a lot more to maintain. But each piece is simple and don't think there will be much maintaining to do. Also, these tokens don't add extra logic to chai (except the switch token). All of this was already supported, the tokens just move the logic for the proper place, the message. My points on why not use only the switch token
About the extra dressingI tried to make it as little as possible while keeping it uniform. The initial design for a token was
But We could drop the
But I think this is easier to read, specially over time, because you get used to look for the
#{this} vs {{this}}We can keep the About
|
Almost all of the syntaxes you add are effectively ternary operations (they assert on value, and depending on truthiness they pick which expression to follow). You've put a bit more complexity in the PluralizeNode - but I think its still lacking. For starters - it only supports 3 switches (0, 1 or more than one). This limits flexibility and, if we were to go down an i18n route - would not be compatible with the myriad of plural forms across various locales (http://unicode.org/repos/cldr-tmp/trunk/diff/supplemental/language_plural_rules.html). You said ternaries could not solve this - but I think they can... bear with me here - and this is only a jumping off point, I'm not prescribing this, just putting it here to discuss further. Adopt Ternary syntaxIf we extend our existing string interpolation - so we have function getInterpolatedParts(string) {
var match = string.match(/#\{([^\?]+)(\?([^:]+)(?:\:(.*))?)?\}/);
return {
property: match[1],
isTernary: match[2] !== undefined,
truthy: match[3] || '',
falsey: match[4] || '',
};
}
getInterpolatedParts('#{foo}').should.eql({ property: 'foo', isTernary: false, truthy: '', falsey: '' });
getInterpolatedParts('#{foo?bar}').should.eql({ property: 'foo', isTernary: true, truthy: 'bar', falsey: '' });
getInterpolatedParts('#{foo?bar:baz}').should.eql({ property: 'foo', isTernary: true, truthy: 'bar', falsey: 'baz' }); Properties support deep notationWe already have code available for parsing deep notation - we can leverage this to enable deep property access within our interpolated strings, meaning Properties support assertionsWe already have code that supports asserting against operators - this could be pulled out into a generic util lib that could be used within chai messages, this way we could do more advanced operations within our interpolated sections, such as Support nested interpolationsWe could support nested interpolations, which could help two fold: one being that we could compose messages out of smaller messages - which themselves could be interpolated, the other being that we could have nested ternaries - which would solve the pluralisation problem much more effectively. See the following two, which support these: Composition: utilMessages = { toBeOrNotToBe: '#{negate?to be:to not be}' };
message = 'expected #{this} #{utilMessages.toBeOrNotToBe} a #{type}';
// expected foo to be a bar
// expected foo to not be a bar Nested Ternary: message = 'expected #{this} to have #{keys.length==0?no:#{keys.length==1?one:many}} #{keys.length==1?key:keys}'
// expected foo to have no keys
// expected foo to have one key
// expected foo to have many keys You could even end up going really crazy with nested ternaries to support very fine grained pluralisation (line breaks & tabs added for readability):
Thoughts @logicalparadox & @svallory? |
Hey @svallory thanks for all of your hard work on this, it has really helped us with where we're headed with chai and your work was amazing. I think I'm going to close this for now though, as it hasn't had much activity recently, and if we go in the direction that #585 is generally pointing in, we may have interesting ways to auto generate messages from assertions. We may come back to this at some point in the future, if and when we do I'll re-open this and let you know @svallory 😄. |
Unfortunately I couldn't finish it today. But enough waiting! Here's a preview of what is going on so you guys can peer review and help me shape it in the best way possible. (original issue: #286)