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

Feature/localization number filters #739

Conversation

jaynarayan89
Copy link
Contributor

Open for review

Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry - this slipped my radar! I thought you were going to be adding more code to the PR, I believe, due to the todos. No biggie.

I've got a few comments here, and we'll also need docs and tests, please. Looks pretty good though. Thanks!

if (! isset($typeValues[$type]))
{
throw new \Exception('syntax error');
//todo: raise mor appropriate /specific exception
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were you going to add more errors here?

static $formatter, $currentStyle;

$locale = $locale ?? \CodeIgniter\Config\Services::request()->getLocale();
log_message('info',\CodeIgniter\Config\Services::request()->getLocale());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like debug code. Please remove.

{
$locale = $locale ?? \CodeIgniter\Config\Services::request()->getLocale();

$formatter = self::getNumberFormatter($locale, 'currency');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation is off.

if (! isset($styleValues[$style]))
{
throw new \Exception('syntax error');
//todo: raise more appropriate /specific exception
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More specific exceptions needed?

{
if (! class_exists('IntlDateFormatter'))
{
throw new \RuntimeException('The intl extension is needed to use intl-based filters.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be localized through the lang() function.

@jaynarayan89
Copy link
Contributor Author

@lonnieezell I was waiting for your reply. Actually after writing this code I notice the format_number function of number helper which I think does the same thing (correct me if I wrong). So I wait before working further.

@lonnieezell
Copy link
Member

Doh! You're right that format_number could be used. Silly me. However, it couldn't be used directly and would need to be called from a filter, since the parameter list is different. And it can throw an exception that you might want to capture, log, and return the original number?

@jaynarayan89
Copy link
Contributor Author

jaynarayan89 commented Oct 21, 2017

@lonnieezell I would like to suggest that we should

i) create a class i18n\number and use that class in number helper and localized filter. This way The NumberFIlters class I had created using twig extension can be used in both files .
ii)Adjust the number_filter helper function to use i18n\number class.

@lonnieezell
Copy link
Member

@jaynarayan89 I'm fine with that.

@lonnieezell
Copy link
Member

@jaynarayan89 Were you going to make these changes you suggested?

@jaynarayan89
Copy link
Contributor Author

@lonnieezell yes I am working on it. Please give me two or three days.

@lonnieezell
Copy link
Member

No worries - just checking in it this had been a few weeks.

@lonnieezell lonnieezell reopened this Nov 13, 2017
@lonnieezell
Copy link
Member

This was never updated. I'm working on a commit for these.

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.

2 participants