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

Allow ISO-8601 to be passed as a format. #1693

Merged
merged 4 commits into from Jun 12, 2014

Conversation

Projects
None yet
2 participants
@sbstp
Contributor

sbstp commented Jun 3, 2014

This commit allows the ISO-8601 format to be passed as a format in the moment(string, array) constructor. This is an attempt to make #1680 possible.This is a preliminary commit as this addition as yielded a few issues. This is not ready for merge.

Here are some of the issues:

  • makeDateFromString does not play nice with scoring (might be due to the way I implemented it)
  • scoring cannot currently be tested because makeDateFromString shoves what it doesn't know into into createFromInputFallback which has been modified to throw always throw an error in the test suite.

I will probably try to add an optional parameter to makeDateFromString which tells it to not use createFromInputFallback to fix the second issue.

@sbstp sbstp changed the title from Allow ISO-8601 to be passed a format. to Allow ISO-8601 to be passed as a format. Jun 3, 2014

sbstp added some commits Jun 3, 2014

Allow ISO-8601 to be passed as a format.
This commit allows the ISO-8601 format to be
passed as a format in the moment(string, array)
constructor. This is a preliminary commit as this
addition as yielded a few issues.
Fix the second issue
I've added a useFallback parameter to makeDateFromString. When not
supplied, the old behavior remains. When supplied and true, it does
not call the createFromInputCallback and instead returns an empty
config. This also seems to fix the first issue, it now plays much
nicer with scoring and precendence.
Add more relevant tests.
I've added much more relevant tests, checking the _f property
for the format that was used for parsing and _pf.iso in the case
of ISO.
@sbstp

This comment has been minimized.

Contributor

sbstp commented Jun 3, 2014

This seems to be working properly, now. It also seems like it didn't break anything else. Let me know if you want me to add more tests or if you think that something won't work properly.

@ichernev

View changes

moment.js Outdated
var i, l,
string = config._i,
match = isoRegex.exec(string);
useFallback = typeof useFallback === 'undefined' ? true : useFallback;

This comment has been minimized.

@ichernev

ichernev Jun 5, 2014

Contributor

useFallback == null should be enough here (check for null or undefined).

@ichernev

View changes

moment.js Outdated
@@ -1822,6 +1833,9 @@
// default format
moment.defaultFormat = isoFormat;
// constant that refers to the ISO standard
moment.ISO_8601 = 'ISO 8601';

This comment has been minimized.

@ichernev

ichernev Jun 5, 2014

Contributor

Here assign an anonymous empty function (function () {}), so it can not be equal to anything else.

@ichernev

View changes

test/moment/create.js Outdated
@@ -395,6 +395,18 @@ exports.create = {
test.equal(moment('01', ["MM", "DD"])._f, "MM", "Should use first valid format");
// pass an ISO date in the array of formats
function parseISO(string) {
return moment(string, [moment.ISO_8601, 'MM', 'HH:mm', 'YYYY']);

This comment has been minimized.

@ichernev

ichernev Jun 5, 2014

Contributor

This looks way too specific (and it isn't just parsing iso). Can you inline this function (improve readability)

@ichernev

View changes

test/moment/create.js Outdated
function parseISO(string) {
return moment(string, [moment.ISO_8601, 'MM', 'HH:mm', 'YYYY']);
}
test.equal(parseISO('1994')._f, 'YYYY', 'iso: test parse YYYY');

This comment has been minimized.

@ichernev

ichernev Jun 5, 2014

Contributor

All the underscore things are not public, also its making the tests very brittle. Just assert that the correct date comes out (do format() and put a full ISO date you want to match against).

@ichernev

View changes

test/moment/create.js Outdated
test.equal(parseISO('06')._f, 'MM', 'iso: test parse MM');
test.equal(parseISO('2012-06-01')._pf.iso, true, 'iso: test parse iso');
test.equal(moment('2014-05-05', [moment.ISO_8601, 'YYYY-MM-DD'])._pf.iso, true, 'iso: edge case array precedence iso');

This comment has been minimized.

@ichernev

ichernev Jun 5, 2014

Contributor

_pf (private) is returned by parsingFlags() (public).

Also I'd put one test where something before ISO matches up, something where ISO matches, skipping non-matching formats, and something after ISO matches.

But the idea is to make it more black box, instead of reading parsing flags and matched format.

@ichernev

View changes

moment.js Outdated
@@ -1456,11 +1461,13 @@
}
// date from iso format
function makeDateFromString(config) {
function makeDateFromString(config, useFallback) {

This comment has been minimized.

@ichernev

ichernev Jun 5, 2014

Contributor

This functions happens to do ISO parsing as well as fallback. I think if we separate out the iso parsing in its own function (which can return if it was successful), then you wouldn't need this useFallback hack.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jun 5, 2014

I I think its good in general. Just please extract the parsing iso part into a separate function.

Implemented suggested changes
`moment.js` now has a separate parseISO function which only handles
the parsing. `makeDateFromString` keeps the same behavior and uses
parseISO. The tests have also been refactored and moved into
their own function.
@sbstp

This comment has been minimized.

Contributor

sbstp commented Jun 5, 2014

Should I also update the docs to document this?

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jun 5, 2014

Thanks, that looks great. About docs you can make a pr at the momentjs.com repo. I do docs changes on release in general but you're welcome to help.

ichernev added a commit that referenced this pull request Jun 12, 2014

Merge pull request #1693 from SBSTP/moment-iso-parse
Allow ISO-8601 to be passed as a format.

@changelog
@Section features
@description support moment.ISO_8601 as a format in constructor, that would use ISO time format parsing

@ichernev ichernev merged commit 31e9f8a into moment:develop Jun 12, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@theurere theurere referenced this pull request Jul 1, 2014

Merged

Update moment.js to 2.7.0 #58

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