Variable, gender and plural addition w/ messageFormat #73
Conversation
|
what are the breaking changes? Can we not keep it backwards compatible without hacky code? It seems to be failing some checks |
|
it's failling because of the default value on options, it's fine It's breaking because of the change on the optionsResult options passing from string to TextResult instance |
|
Now we need to changes all of ./tests/test_runner.js for compat because of the new TextResult.formater property |
|
@blurymind i've a problem with tests because of the instance of formater witch is never deeply equal I found a "kinda" way-out by checking keys instead for data itself but i don't think i'ts a good solution to that |
I assume the tests that check for equality will need to be updated. Can you add an example json file of formatter usage to https://github.com/hylyh/bondage.js/tree/master/tests/yarn_files |
.travis.yml
Outdated
| - "4.1" | ||
| - "4.0" | ||
| - node | ||
| - 8.3 |
There was a problem hiding this comment.
I wonder if updating this travis file like this has an effect on the github tests
There was a problem hiding this comment.
i needed to update because i uses spread operator so tests crashed every time
There was a problem hiding this comment.
ah I see, that is worrying - perhaps we need to revert this and not use deconstruction in that one place - as it will crash bondagejs on some older browsers. Having it still work on ancient browsers is nice and if its easy to avoid losing this, its worth avoiding it
There was a problem hiding this comment.
Can we revert the changes to this file now? Tests should pass as we removed the spread operator
|
Sorry I left a bunch of notes :) I want to reduce the changes in the pr to make it safer (no breaking changes) and also reduce the work we need to do to fix these tests. There are a few unnecessary things here that can be done in some other pr or not done at all (such as attaching the formater to node results). Can you refactor these please, should be easy to do. I believe it will fix your failing tests too. From there on we can add a bunch of new formater related tests |
|
No problem |
|
Everything is done ! |
Thank you ^_^ I think the user can still override the formater by setting the runner's formater when initializing bondagejs. This is much simpler than attaching a formater on each result node. We now just need to write a simple example json file that demonstrates the formater in a text result and in an option result and a simple equality test that checks if it does its job. Bondage has lots of already existing tests that we can use as example, a single one would make me very happy - even if its really basic |
tests/test_runner.js
Outdated
| expect(value.text).to.equal('You have one piece inside your pocket.'); | ||
| run = runner.run('HighMoney'); | ||
| value = run.next().value; | ||
| expect(value.text).to.equal('You have 175 gold pieces!'); |
There was a problem hiding this comment.
ah nice! You did add the tests 👏 🎉 Thank you! Wow we might be able to merge this today!
There was a problem hiding this comment.
i don't added the tests for time and duration yet, i'll do that later on
tests/yarn_files/formatting.json
Outdated
| { | ||
| "title": "Gender", | ||
| "tags": "", | ||
| "body": "Note: {gender, select, male {He} female {She} other {They}}'ll remember that.", |
There was a problem hiding this comment.
this formatting is slightly different than
https://yarnspinner.dev/docs/syntax#expression
I haven't seen [select {$gender} m="him" f="her" nb="them"].
but at least it gives us feature parity. I wonder if we can adjust MessageFormat's text parsing later on.
I am happy with just having the feature available to js users for now,so dont worry about it in this PR if it requires a lot of work
There was a problem hiding this comment.
because it's using the naming convention for var, so in C# it's $var, but in js it's just var
There was a problem hiding this comment.
i saw that we can make custom template on the lib, i'll try looking at the documentation
There was a problem hiding this comment.
I wonder if yarnSpinner devs set it up in a special way in their source code, or this just comes from the formater being a javascript implementation
There was a problem hiding this comment.
I saw that this lib is on a lot of language, maybe devs wanted to implement it with the appropriate syntax following the language
There was a problem hiding this comment.
This is the commit which added format functions to yarnSpinner
YarnSpinnerTool/YarnSpinner@c6e3756
maybe it can help to figure out what they did
There was a problem hiding this comment.
Posted a question about this, maybe the devs can help us here
messageformat/messageformat#318
src/runner.js
Outdated
| @@ -14,14 +14,12 @@ class Runner { | |||
| * @param {string} [options.language] Options for the language of the formater | |||
| * @memberof Runner | |||
| */ | |||
| constructor(options) { | |||
| constructor(options = { language: 'en' }) { | |||
There was a problem hiding this comment.
if options is empty object, will cause option.language to be undefined too
There was a problem hiding this comment.
another way to prevent that, on ln22 pass new MessageFormat(options.language) to new MessageFormat(options.language || 'en') ?
There was a problem hiding this comment.
trying to get options.language when options is undefined by default will fail.
You would still need to do options = { }
|
Ok so it looks like this is more work to it than meets the eye. The question is do we want to go as far as give this its own result node type? Then how do we get the syntax - they seem to do something to the string before sending it off to be formated - so we would need to do the lexer stuff to identify this as a string that needs formatting in the first place. We cant just send text/options to the formater. We need to register another node type for this, then do some operations to the text in it when its encountered before we send it to MessageFormat |
/!\ Include breaking changes