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

[DT] Added to parse the same optional config argument that format uses. #587

Closed
wants to merge 24 commits into from

Conversation

Satyam
Copy link
Contributor

@Satyam Satyam commented Apr 5, 2013

Added tests, updated the API docs, user guide and example.

@@ -6,19 +6,54 @@
* @for Number
*/

var LANG = Y.Lang;
var LANG = Y.Lang,
safeRegExp = /(\\|\[|\]|\.|\+|\*|\?|\^|\$|\(|\)|\||\{|\})/g;
Copy link
Contributor

Choose a reason for hiding this comment

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

This desperately needs a comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can also get the regex under 2 inches with a character class:

var LANG = Y.Lang,
    safeRegExp;

// Matches: \ [ ] . + * ? ^ $ ( ) | { }
safeRegExp = /[\\[\].+*?\^$()|{}]/g

Copy link
Contributor

Choose a reason for hiding this comment

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

0 inches by using Y.Escape.regex() instead! :D

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@Satyam
Copy link
Contributor Author

Satyam commented Apr 19, 2013

Thanks guys!!!! Brilliant advice. Luke's gist is not just a general sketch, it works wonderfully. Thanks.

I wasn't thinking about batch conversions such as in a DataSchema. Since my need came from the inline cell editor, I was thinking of one-at-a-time use where speed is hardly critical.

I'll look into the date parser as well to apply the same idea.

@Satyam
Copy link
Contributor Author

Satyam commented Apr 19, 2013

Question:_Should I apply the same idea of caching to the number formatter. That one sure gets called repeatedly far more often than the parser. What do you think?

@mattparker
Copy link

When the cached parser is created, could it be added as a property to the function, so that it's available if it needs to be modified (e.g. I don't know, handle exponents 2.3 e10 or something). As it is it's a bit like true privates in classes, which I thought was generally best avoided? Something like this: https://gist.github.com/mattparker/5420634 ?

@lsmith
Copy link
Contributor

lsmith commented Apr 19, 2013

@Satyam, Thanks :) Yes, definitely cache the number parsers as well. My primary concern with using Y.cached is the memory allocation due to the inability to clear the cache. But I'm guessing it's unlikely a page will use lots of number formats, so I left it alone.

@mattparker buildParser is a cached function. It will return the same function for the same inputs, but will create a new one if new inputs are passed in. Your gist reuses the first built parser, regardless of future configs being different (bug). I can see the argument to assign the buildParser function to a protected/private property of Type.Number and referencing it as Number._buildParser(...) in the parse function. That would allow for customizing or extending the behavior used to actually parse the number.

@mattparker
Copy link

Sorry Luke. I read your gist and thought "ooh I should read about Y.Cached"... perhaps I should've first... ahem. Anyway having it available would be good, even more so for dates where I can see you are much more likely to want to be able to specify other formats.

I have to pay attention to JSHint messages.
.... and so on.
@Satyam
Copy link
Contributor Author

Satyam commented Apr 19, 2013

@mattparker, @lsmith: Actually, I changed the inaccessible buildParser into a static private method and it works so it can both be redefined and still be cached. I was concerned it wouldn't work, however, I put a console.log statement inside showing when the inner function was actually called, not its cached version, and it does get called just once for each set of arguments so it works fine.

Probably it works because these are static functions. Nothing in the cached function makes any reference to the object state information since, being all static, there is no such thing.

I was also concerned about a possible unbound growth of the cache, but I also figured out the number of possible formats would be very small in any actual application. My guess is that there would be only one currency format for the whole application and possibly another for regular numbers for those of us who use comma as the decimal separator and that would be it, at least in most cases. I would bet that doing the same for dates would also result in very few cache entries.

@lsmith
Copy link
Contributor

lsmith commented Apr 19, 2013

@Satyam, My thinking exactly :)

@Satyam
Copy link
Contributor Author

Satyam commented Apr 19, 2013

I tried to cache the number formatter in a way similar to the number parser, but there is really very little that can be cached. The worst part is the thousands separator, which cuts and shuffles parts of the string to place the separators in between, taking care not to touch the decimal part or the sign. All that logic depends on the value being formatted, not on the formatting configuration so there is very little that can be prepared beforehand and cached.

While doing all this optimizing of the thousands separator part (I thought I had a better way, but it was not the case) I was thinking that we have no options for lakh and crore. At least in India (possibly in other countries, but I'm not acquainted with much of the region), after the first thousands separator, the separator gets repeated every two positions, not every three as we do, like this: 1,23,45,67,890.-- instead of 1,234,567,890.--

I don't know how it feels to English speakers but in Spanish we don't even have the option of saying things like 'fifteen hundred', in Spanish it is always 'one thousand five hundred'. Thus, we are pretty used to split things every three digits, our language offers no alternative as English does.

That is why the Indian way of separating large numbers into hundreds and then hundreds of hundreds after the first thousand is an even more foreign way of looking at numbers.

My next piece of counting weirdness will be about how to say the time in Catalonia. If you ever thought that date arithmetic was tough, just wait. A company has even manufactured a watch that spells the time in Catalan: http://www.horacat.cat/

But that's going to be some other day. I wonder, how do I get into these strange places? Oh dear!

@lsmith
Copy link
Contributor

lsmith commented Apr 19, 2013

I don't think there's ever been support for formatting numbers like that, so I'm not worried about that impacting the library.

Also, wow. I had no idea :)

While trying to write a better version I found some of the
tests didn't cover some edge conditions and they didn't
check the groups of numbers were not reversed
or shuffled around.
@Satyam
Copy link
Contributor Author

Satyam commented Apr 21, 2013

@lsmith: I applied caching to Y.Date.parse as well: https://github.com/yui/yui3/pull/566/files#L5R406

@apipkin
Copy link
Contributor

apipkin commented May 21, 2013

This looks really good to me (pending passing tests/builds).

@lsmith do you have any other thoughts?

@ericf
Copy link
Member

ericf commented May 21, 2013

Needs a HISTORY.md entry.


var testParseWithConfig = new Y.Test.Case({
name: "Number Parse w/ Config Tests",
tests: function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

More descriptive name, please. You probably don't need to separate this out to its own TestCase.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • Add at least one test for the failure case where parse() returns null.

@lsmith
Copy link
Contributor

lsmith commented May 22, 2013

Thanks for the HISTORY entry!

Corrected as per @lsmith so that a string of nothing but whitespace
now returns null and not 0 as before.
Changed tests to account for this change.
@Satyam
Copy link
Contributor Author

Satyam commented May 25, 2013

@lsmith There is this strange thing about filling the history.md file. I tried to follow the format but it doesn't really feel right. I put 3.10.2 as the version, but I can't be sure. Was that the right version? You don't really know until it gets out through the door. It would be nice to be able to fill out the history.md when you first post the PR but then, you don't know what version to put there, this one was originally posted two months ago, I don't know which version was upcoming back then, certainly not the current. I don't know, the whole history.md thing feels strange.

@lsmith
Copy link
Contributor

lsmith commented May 26, 2013

You can use @Version@ in the HISTORY.md files.

@Satyam
Copy link
Contributor Author

Satyam commented May 26, 2013

So, at some point after the version is released the earlier versions get converted into a fixed string? If so, it would be good if the documentation (https://github.com/yui/yui3/wiki/Pull-Request-Flow#5-edit-the-historymd-files-for-any-modules-you-changed-optional) said so, otherwise, you just go by whatever you see in the unmodified file and follow it as close as possible.

@apipkin apipkin closed this May 27, 2013
@apipkin apipkin reopened this May 27, 2013
@triptych
Copy link
Contributor

FYI @Satyam I updated the documentation. Thanks!

@ericf
Copy link
Member

ericf commented Jul 19, 2013

This seems to include unintended changes to files which are outside the scope of this PR.

@triptych
Copy link
Contributor

@Satyam do you want to close this one out and issue a new PR with your changes w/out the other unintended changes? Or are you going to update this PR?

@Satyam
Copy link
Contributor Author

Satyam commented Aug 30, 2013

@triptych : Honestly, none of the above. Look at the activity this and my several other PRs show. My efforts have been passed by. I have mentioned elsewhere how much trouble I had having to work in a PR dependent on other PRs not yet merged, forced to juggle source files around. Of course, none of that would have happened if my PRs had been processed in a reasonable time. Two full releases have gone by while this was pending, how can I cope with all the merges that brought into my branches?

Should I put any more effort on this? Why? What do you have on offer? Look at the heading at the top "no one is assigned", "No milestone", "opened 5 months ago". Will that change? I don't see that happening, except for the months piling up. Just as I am replying to you in a timely manner, I acted on upon anything requested from me in a timeline way (except for one case where I simply didn't know how to reproduce the issue, which I fixed as soon as someone gave me the clue). Then, they languish again for weeks. Finaly, I gave up, it would have been foolish to bang my head against the wall any more.

My guess is that you just mean to clean the backlog of pending PRs. Feel free to. Just don't put the blame on me. I worked hard and I think my additions are useful. I simply had to face the fact that no one in a position of letting them go through cares (actually, someone said as much in a YUI roundtable).

So, don't ask me what do I intend to do, tell me what do you intend to do and then we might strike a deal.(not you personnally, but since you are asking, I assume you have a stake on deciding) I have already done more than my fair share, what do you have on offer? Sorry if it sounds like a challenge, it is not meant to be. It is simply that at this point I can no longer offer to blindly put any more effort into it, At this point I can only say 'I'm listening'.

@apipkin
Copy link
Contributor

apipkin commented Sep 27, 2013

Closing this PR in favor of PR #1244

@apipkin apipkin closed this Sep 27, 2013
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.

8 participants