Skip to content
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

Improve error when translation is missing for a given key #40

Merged
merged 2 commits into from
Mar 20, 2019

Conversation

viniciusoyama
Copy link
Contributor

Proposed changes

Today, if I try to fetch a message passing the wrong key I get an error from the intl-messageformat lib:

> const Antl = use('Antl')
> Antl.formatMessage('messages.greeting')
TypeError: A message must be provided as a String or AST.
    at new MessageFormat (/Users/viny/Projects/fleury-checkup-executivo/node_modules/intl-messageformat/lib/core.js:21:15)
    at /Users/viny/Projects/fleury-checkup-executivo/node_modules/intl-format-cache/lib/memoizer.js:24:22
    at Formatter.formatMessage (/Users/viny/Projects/fleury-checkup-executivo/node_modules/@adonisjs/antl/src/Formatter/index.js:177:12)
    at Antl.formatMessage (/Users/viny/Projects/fleury-checkup-executivo/node_modules/@adonisjs/antl/src/Antl/index.js:113:28)
    at repl:1:6
    at Script.runInContext (vm.js:107:20)
    at REPLServer.defaultEval (repl.js:331:29)
    at bound (domain.js:396:14)
    at REPLServer.runBound (domain.js:409:12)

I've changed the code so we can have a more descriptive error containing the current locale and key:

InvalidArgumentException: E_INVALID_PARAMETER: Missing pt translation for key 'messages.greeting'

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have read the CONTRIBUTING doc
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate)

Further comments

Maybe we should create some integration tests. This is a case that the suite was not covering...

@viniciusoyama viniciusoyama changed the title Improve error in #formatMessage when translation is missing for a given key Improve error when translation is missing for a given key Mar 19, 2019
@coveralls
Copy link

coveralls commented Mar 19, 2019

Coverage Status

Coverage increased (+0.08%) to 96.0% when pulling e37d19e on viniciusoyama:develop into e54c93f on adonisjs:develop.

@thetutlage
Copy link
Member

You are changing the functionality of the get method, which introduces an unnecessary breaking change for applications expecting null.

Instead, you should change formatMessage to throw a more helpful error

@viniciusoyama
Copy link
Contributor Author

Hi. I can do that but I thought about implementing like you said before and I think that the get() method it's more private than public.

I couldn't find any documentation about that on the AdonisJS documentation so no one should be using it.

Even if there is someone using the get method, expecting it to return null I see as an anti-pattern and it's very likely to end in a case where there is some kind of typo and the developer will never know that there is an error because the method is returning null.

Of course that I can understand your point so I will do what you think it's better 👍

@viniciusoyama viniciusoyama force-pushed the develop branch 2 times, most recently from e1fcc56 to 363a1b3 Compare March 20, 2019 03:48
@viniciusoyama
Copy link
Contributor Author

Done! I've also added a test for the formatMessage method.

Thanks for the quick feedback. 👍

@thetutlage
Copy link
Member

Looks great :)

@thetutlage thetutlage merged commit 0b22a04 into adonisjs:develop Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants