feat(common): make date/number formatting functions public #22372

ocombe · 2018-02-22T16:07:32Z

PR Type

What kind of change does this PR introduce?

[x] Feature

What is the current behavior?

If you want to use the number, percent, currency and date pipes outside of your templates, you need to instantiate those pipes and manually transform the values

Issue Number: #20536

What is the new behavior?

The utility functions formatNumber, formatPercent, formatCurrency, and formatDate used by the number, percent, currency and date pipes are now available for developers who want to use them outside of templates.

Does this PR introduce a breaking change?

[x] No

mary-poppins · 2018-02-22T16:17:23Z

You can preview aee9a42 at https://pr22372-aee9a42.ngbuilds.io/.

mary-poppins · 2018-02-22T16:39:47Z

You can preview c4e518f at https://pr22372-c4e518f.ngbuilds.io/.

vicb · 2018-02-22T17:21:08Z

packages/common/src/i18n/format_date.ts

@@ -40,25 +43,26 @@ enum TranslationType {
 /**
 * Transforms a date to a locale string based on a pattern and a timezone
 *
- * @internal
+ * @stable


needs more doc !

vicb · 2018-02-22T17:22:03Z

packages/common/src/i18n/format_date.ts

@@ -607,3 +613,66 @@ function convertTimezoneToLocal(date: Date, timezone: string, reverse: boolean):
  const timezoneOffset = timezoneToOffset(timezone, dateTimezoneOffset);
  return addDateMinutes(date, reverseValue * (timezoneOffset - dateTimezoneOffset));
 }
+
+/**
+ * Transforms a value into a date


comment here do no say anything more than toDate().
Pls improve

refine type also

vicb · 2018-02-22T17:22:41Z

packages/common/src/i18n/format_date.ts

+    return new Date(parseFloat(value));
+  } else if (typeof value === 'string' && /^(\d{4}-\d{1,2}-\d{1,2})$/.test(value)) {
+    /**
+     * For ISO Strings without time the day, month and year must be extracted from the ISO String


move to API doc of the function ?

No, it's there to explain the method used to convert the ISO string to a date, but it has no value for people who use the function (also keep in mind that the function is internal only for now).

vicb · 2018-02-22T17:23:14Z

packages/common/src/i18n/format_date.ts

+  return date;
+}
+
+export function isoStringToDate(match: RegExpMatchArray): Date {


add a comment here pls

vicb · 2018-02-22T17:23:55Z

packages/common/src/i18n/format_number.ts

@@ -140,7 +140,7 @@ function formatNumber(
 /**
 * Formats a currency to a locale string
 *


please add docs

vicb · 2018-02-22T17:24:05Z

packages/common/src/i18n/format_number.ts

@@ -162,12 +162,12 @@ export function formatCurrency(
 /**
 * Formats a percentage to a locale string
 *


please add docs

vicb · 2018-02-22T17:24:50Z

packages/common/src/pipes/date_pipe.ts

@@ -120,6 +116,9 @@ export const ISO8601_DATE_REGEX =
 *   pipe needs to re-run (this is to avoid reformatting the date on every change detection run
 *   which would be an expensive operation).
 *
+ * If you want to use this pipe outside of a template, you can use the function


no you don't want to use the pipe outside of a template. Remove ?

vicb · 2018-02-22T17:25:12Z

packages/common/src/pipes/number_pipe.ts

@@ -31,6 +31,9 @@ import {invalidPipeArgumentError} from './invalid_pipe_argument_error';
 * For more information on the acceptable range for each of these numbers and other
 * details see your native internationalization library.
 *
+ * If you want to use this pipe outside of a template, you can use the function


same comment as other non-pipes

vicb · 2018-02-22T17:26:17Z

packages/common/test/pipes/date_pipe_spec.ts

@@ -47,28 +48,44 @@ import localeAr from '@angular/common/locales/ar';
    });

    describe('supports', () => {
-      it('should support date', () => { expect(() => pipe.transform(date)).not.toThrow(); });
+      it('should support date', () => {
+        expect(isDate(toDate(date))).toBeTruthy();


what is the intent here ?
Does this belong to date_pipe_spec.ts ?

vicb

Please

Add or expand the API docs. Especially important for API docs. Internal functions should also have a short comment,
refine date type from "any" to a|b|c,
I was expected to see more tests moved to the new format spec. It doesn't make sense any more to have them in the pipes if pipes only forward to those functions (only a sanity check is ok for the pipes).

mary-poppins · 2018-02-23T17:20:23Z

You can preview 0fe525a at https://pr22372-0fe525a.ngbuilds.io/.

mary-poppins · 2018-02-23T17:25:03Z

You can preview d4d8baa at https://pr22372-d4d8baa.ngbuilds.io/.

mary-poppins · 2018-02-23T17:25:29Z

You can preview a3fd774 at https://pr22372-a3fd774.ngbuilds.io/.

vicb · 2018-02-23T17:46:41Z

packages/common/src/i18n/format_date.ts

 *
- * @internal
+ * Where:
+ * - `value` is a date object or a number (milliseconds since UTC epoch) or an ISO string


please update the type accordingly

"is a Date or"

vicb · 2018-02-23T17:48:35Z

packages/common/src/i18n/format_date.ts

+ * (https://www.w3.org/TR/NOTE-datetime).
+ * - `format` indicates which date/time components to include. See {@link DatePipe} for a detailed
+ * description.
+ * - `locale` is a `string` defining the locale to use (uses the current {@link LOCALE_ID} by


vicb · 2018-02-23T17:50:41Z

packages/common/src/i18n/format_date.ts

+ * default).
+ * - `timezone` to be used for formatting. It understands UTC/GMT and the continental US time zone
+ *  abbreviations, but for general use, use a time zone offset, for example,
+ *  `'+0430'` (4 hours, 30 minutes east of the Greenwich meridian)


remove "(4 hours, 30 minutes east of the Greenwich meridian)" - no added value and technically wrong GMT vs UTC

vicb · 2018-02-23T19:11:18Z

packages/common/src/i18n/format_date.ts

@@ -607,3 +628,90 @@ function convertTimezoneToLocal(date: Date, timezone: string, reverse: boolean):
  const timezoneOffset = timezoneToOffset(timezone, dateTimezoneOffset);
  return addDateMinutes(date, reverseValue * (timezoneOffset - dateTimezoneOffset));
 }
+
+/**
+ * Converts a value to date


missing final .

vicb · 2018-02-23T19:12:09Z

packages/common/src/i18n/format_date.ts

+}
+
+/**
+ * Convert a date in ISO8601 to a Date.


vicb · 2018-02-23T19:12:34Z

packages/common/src/i18n/format_date.ts

+
+/**
+ * Convert a date in ISO8601 to a Date.
+ * Used instead of Date.parse because of browser discrepancies.


quote Date.parse

vicb · 2018-02-23T19:13:04Z

packages/common/src/i18n/format_number.ts

+ * Use `currency` to format a number as currency.
+ *
+ * Where:
+ * - `value` is a number or numeric string.


wrong way ! -> number

vicb · 2018-02-23T19:14:16Z

packages/common/src/i18n/format_number.ts

+ * Where:
+ * - `value` is a number or numeric string.
+ * - `locale` is a `string` defining the locale to use.
+ * - `digitInfo` is a `string` which has a following format: <br>


add to all pipes or no pipe (other have {@link ...})

mary-poppins · 2018-02-23T19:24:39Z

You can preview da8586b at https://pr22372-da8586b.ngbuilds.io/.

wKoza · 2018-02-23T19:50:30Z

packages/common/src/i18n/format_date.ts

+ *   (https://www.w3.org/TR/NOTE-datetime).
+ * - `format` indicates which date/time components to include. See {@link DatePipe} for more
+ *   details.
+ * - `locale` is a `string` defining the locale to use


missing final .

wKoza · 2018-02-23T20:05:41Z

packages/common/src/i18n/format_date.ts

+  let strNum = String(num);
+  while (strNum.length < digits) {
+    strNum = '0' + strNum;
+  }
  if (trim) {
    strNum = strNum.substr(strNum.length - digits);


strNum.length - digits isn't always equal to 0?

not always, we can have additional zeros

wKoza · 2018-02-23T20:08:50Z

packages/common/src/i18n/format_date.ts

+ * Converts a value to date
+ *
+ * Supported input formats:
+ * - `Date`s


mary-poppins · 2018-02-23T21:06:42Z

You can preview 6d02858 at https://pr22372-6d02858.ngbuilds.io/.

ngbot · 2018-02-23T21:11:05Z

I see that you just added the PR action: merge label, but the following checks are still failing:
     status "ci/circleci: build" is failing
     status "ci/circleci: lint" is failing
     status "code-review/pullapprove" is pending
     status "google3" is pending
     status "continuous-integration/travis-ci/pr" is pending
     1 pending code review

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

…es & dates The utility functions `formatNumber`, `formatPercent`, `formatCurrency`, and `formatDate` used by the number, percent, currency and date pipes are now available for developers who want to use them outside of templates. Fixes angular#20536

mary-poppins · 2018-02-23T21:37:52Z

You can preview dc77b4e at https://pr22372-dc77b4e.ngbuilds.io/.

IgorMinar

the rest looks good. thanks

IgorMinar · 2018-02-23T21:42:59Z

packages/common/src/i18n/format_number.ts

 /**
 * Transforms a number to a locale string based on a style and a format
 */
-function formatNumber(
-    value: number | string, pattern: ParsedNumberFormat, locale: string, groupSymbol: NumberSymbol,
+function formatNbToLocaleString(


please avoid using custom abbreviations in non-local variables. formatNbToLocaleString -> formatNumberToLocaleString

anyone not familiar with our codebase that sees "Nb" will have to question what it means. This makes our code base harder to read and change by people not familiar with every change we make - e.g. our community, but also team members working on other parts of the code base that come across this code.

…rrencies & dates

ngbot · 2018-02-23T22:22:01Z

I see that you just added the PR action: merge label, but the following checks are still failing:
     status "ci/circleci: lint" is failing
     status "google3" is pending
     status "continuous-integration/travis-ci/pr" is pending
     1 pending code review

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

vicb · 2018-02-23T22:22:18Z

presubmit

mary-poppins · 2018-02-23T22:24:54Z

You can preview 8ce1f81 at https://pr22372-8ce1f81.ngbuilds.io/.

angular-automatic-lock-bot · 2019-09-13T08:52:10Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

ocombe added feature Issue that requests a new feature action: review The PR is still awaiting reviews from at least one requested reviewer area: common Issues related to APIs in the @angular/common package target: major This PR is targeted for the next major release labels Feb 22, 2018

ocombe added this to the v6-candidates milestone Feb 22, 2018

ocombe assigned vicb Feb 22, 2018

ocombe requested a review from vicb February 22, 2018 16:07

googlebot added the cla: yes label Feb 22, 2018

ocombe force-pushed the feat/#20536-export-pipe-fcts branch from aee9a42 to c4e518f Compare February 22, 2018 16:31

vicb reviewed Feb 22, 2018

View reviewed changes

vicb suggested changes Feb 22, 2018

View reviewed changes

vicb changed the title ~~feat(common): export pipe utility functions~~ feat(common): make date/number formatting functions public Feb 22, 2018

ocombe force-pushed the feat/#20536-export-pipe-fcts branch 2 times, most recently from d4d8baa to a3fd774 Compare February 23, 2018 17:17

vicb reviewed Feb 23, 2018

View reviewed changes

packages/common/src/i18n/format_date.ts Outdated

}

/**

* Convert a date in ISO8601 to a Date.

Copy link

Contributor

vicb Feb 23, 2018

Choose a reason for hiding this comment

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

Converts

vicb reviewed Feb 23, 2018

View reviewed changes

ocombe force-pushed the feat/#20536-export-pipe-fcts branch from 68fe313 to da8586b Compare February 23, 2018 19:13

vicb reviewed Feb 23, 2018

View reviewed changes

wKoza reviewed Feb 23, 2018

View reviewed changes

vicb added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 23, 2018

ocombe force-pushed the feat/#20536-export-pipe-fcts branch from 6d02858 to dc77b4e Compare February 23, 2018 21:29

IgorMinar approved these changes Feb 23, 2018

View reviewed changes

IgorMinar added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Feb 23, 2018

fixup! feat(common): export functions to format numbers, percents, cu…

8ce1f81

…rrencies & dates

vicb approved these changes Feb 23, 2018

View reviewed changes

vicb added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Feb 23, 2018

vicb mentioned this pull request Feb 23, 2018

feat(common): export functions to format numbers, percents, currencie… #22423

Closed

vicb closed this Feb 23, 2018

ocombe deleted the feat/#20536-export-pipe-fcts branch February 24, 2018 08:18

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

feat(common): make date/number formatting functions public #22372

feat(common): make date/number formatting functions public #22372

Conversation

ocombe commented Feb 22, 2018

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

mary-poppins commented Feb 22, 2018

mary-poppins commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocombe Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb left a comment

Choose a reason for hiding this comment

mary-poppins commented Feb 23, 2018

mary-poppins commented Feb 23, 2018

mary-poppins commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Feb 23, 2018

ngbot bot commented Feb 23, 2018

mary-poppins commented Feb 23, 2018

IgorMinar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngbot bot commented Feb 23, 2018

vicb commented Feb 23, 2018

mary-poppins commented Feb 23, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

ocombe Feb 23, 2018 •

edited

Loading