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

Date and Number formatting #362

Closed
wants to merge 2 commits into from
Closed

Conversation

ashiksmd
Copy link

As requested, branched with only the code, along with just the root locale data. Extra locale data has been removed.
(See also pr #348 which this is derived from.)

//Exceptions

Y.mix(Format, {
Exception: function(name, message) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't define custom exception classes. This is widely considered bad practice in JS. In YUI, you should use Y.error().

@mattparker
Copy link

For info: I've been working on parsing date/time strings into timestamps, along with relative changes - basically a full implementation of php's strtotime(), but with more (ie some) Internationalization support.

It's some way from being finished, but is https://github.com/mattparker/strtotime I intend it to go in the new gallery, eventually. The README will tell you what it does (and it does what it says): but I wouldn't suggest spending any time reading the code because there's a bunch of changes I want to make still (the code and I are on a break at the moment!).

More directly related to this: one thing that it occurred to me to do after the last lot of commits to strotime is to strip out timezone support (strtotime parses string timestamps, of which there are a lot), and which also appear here. And there's timezone string info (which could be significantly minified, incidentally) - so perhaps timezone data needs to be pulled out.

Other general comments on this PR: it's not at all clear what this does that Y.Date.format doesn't. I'm not actually very clear what this does do at all. It doesn't read like javascript.

@rgrove
Copy link
Contributor

rgrove commented Jan 14, 2013

-1

This code makes no apparent effort to adhere to YUI best practices or coding standards. It hasn't even been linted. It is written in a verbose, repetitive, copy/paste-heavy, Java-like style that doesn't reflect modern JavaScript conventions. For example, in multiple places it modifies String.prototype to add a trim() method, which is not only a violation of fundamental good practice, it's also unnecessary because YUI has Y.Lang.trim().

Furthermore, the code is poorly commented, undocumented, and is presented here in a monolithic pull request with no description. It is virtually impossible to review, and my line comments above are merely a surface skim.

This should not go into YUI in this form or in anything like this form.

@davglass
Copy link
Member

After reading (or trying to read) this code, I'm a 👎 on this until it get's cleaned up.

There are several design flaws in this that make it unacceptable to merge in this format.

The String.prototype stuff is easy to replace and do properly, but there are very large parts of this code that just look "wrong" and look like they are a direct port from Java.

* @constructor
* @param {Number} style (Optional) Selector for the desired relative time format. If no argument is provided, default to ONE_UNIT_LONG. If argument is provided but is not a valid selector, an Error exception is thrown.
*/
YRelativeTimeFormat = function(style) {
Copy link
Member

Choose a reason for hiding this comment

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

Global Variable.

return;
}
var head, tail, segment;
for (var i = 0; i < pattern.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Bad code smell here, 2 while loops, 2 for loops, continue's and break's just smells bad to me.

@davglass
Copy link
Member

I pulled this in locally and I can't even get the tests to pass. It needs linting very badly and it needs it globals checked. I found way too may leaking vars.

@davglass
Copy link
Member

Just adding here for posterity: yogi [warn] lint found a total of 1063 errors.

@davglass
Copy link
Member

Forgot to mention that this all needs YUIDoc comments added to every function.

@davglass
Copy link
Member

Closed based on Open Hangout, recommending that this move into Gallery.

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.

5 participants