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

feat(datePipe): numeric string support #9124

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@laskoviymishka
Contributor

laskoviymishka commented Jun 9, 2016

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

Numeric string is not supported.

What is the new behavior?

Numeric string would successfully transformed by date pipe.

Does this PR introduce a breaking change?

  • Yes
  • No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

This is really small PR. I not sure does this behavior should exist, but ng1 work with numeric strings.
Related to #3261

cc @mhevery

@googlebot googlebot added the cla: yes label Jun 9, 2016

@@ -320,6 +320,8 @@ export class NumberWrapper {
static get NaN(): number { return NaN; }
static isNumeric(value: any): boolean { return !isNaN(parseInt(value)); }

This comment has been minimized.

@vicb

vicb Jun 9, 2016

Contributor

isNumeric("0a") = true
Could you please add tests

This comment has been minimized.

@laskoviymishka

laskoviymishka Jun 10, 2016

Contributor

sorry, my bad, it should be false.
Test was added, and this check fixed.

@laskoviymishka

This comment has been minimized.

Contributor

laskoviymishka commented Jun 10, 2016

@vicb address comments, merge to latest, cleanup and squash

describe('Number', () => {
describe('isNumeric', () => {
it('should return true when passing correct numeric string',
() => { expect(NumberWrapper.isNumeric('2')).toBe(true); });

This comment has been minimized.

@vicb

vicb Jun 10, 2016

Contributor

1.123, 1e5, negative values

@@ -48,7 +47,7 @@ var defaultLocale: string = 'en-US';
* punctuations, ...) and details of the formatting will be dependent on the locale.
* On the other hand in Dart version, you can also include quoted text as well as some extra
* date/time components such as quarter. For more information see:
* https://www.dartdocs.org/documentation/intl/0.13.0/intl/DateFormat-class.html
* https://api.dartlang.org/apidocs/channels/stable/dartdoc-viewer/intl/intl.DateFormat.

This comment has been minimized.

@vicb

vicb Jun 10, 2016

Contributor

I think this should be reverted

@vicb

This comment has been minimized.

Contributor

vicb commented Jun 10, 2016

you also need to rebase and run gulp format on your PR, thanks

@laskoviymishka

This comment has been minimized.

Contributor

laskoviymishka commented Jun 15, 2016

@vicb thanks for response, done, please review.

@@ -21,6 +21,9 @@ export function main() {
describe('supports', () => {
it('should support date', () => { expect(pipe.supports(date)).toBe(true); });
it('should support int', () => { expect(pipe.supports(123456789)).toBe(true); });
it('should support numeric strings',
() => { expect(pipe.supports('123456789')).toBe(true); });

This comment has been minimized.

@vicb

vicb Jun 17, 2016

Contributor

supports is actually private, see the master version

This comment has been minimized.

@laskoviymishka

laskoviymishka Jun 17, 2016

Contributor

Damn, it escalate quickly

@vicb

This comment has been minimized.

Contributor

vicb commented Jun 17, 2016

Thanks for the update, could you please rebase and then I'll merge. Sorry for the delay.

@laskoviymishka

This comment has been minimized.

Contributor

laskoviymishka commented Jun 17, 2016

Done, please review

@vicb

This comment has been minimized.

Contributor

vicb commented Jun 17, 2016

Thanks !

@vicb

This comment has been minimized.

Contributor

vicb commented Jun 17, 2016

merged via 5c8d315

@vicb vicb closed this Jun 17, 2016

@laskoviymishka laskoviymishka deleted the laskoviymishka:feat/datePipeNumeric branch Jun 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment