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

Don't generate invalid dates #840

Closed
wants to merge 3 commits into from
Closed

Don't generate invalid dates #840

wants to merge 3 commits into from

Conversation

mredbishop
Copy link

Don't generate invalid dates when the schema uses a date time format that is incompatible with the javascript Date object constructor such as '1987-01-01-05:00'

Don't generate invalid dates when the schema uses a date time format that is incompatible with the javascript Date object constructor such as '1987-01-01-05:00'
@coveralls
Copy link

coveralls commented May 17, 2016

Coverage Status

Coverage decreased (-0.05%) to 92.097% when pulling 09e7018 on mredbishop:patch-1 into 2c9c537 on vpulim:master.

@herom
Copy link
Contributor

herom commented May 17, 2016

Thanks a lot!

Perhaps we should consider to add moment.js as a dependency and allow users to pass a format string for date conversion? Would you take on the challenge to implement it?

If not, I'll be happy to merge your request but I must ask for at least 1 test for this too as is stated in our CONTRIBUTING.md.

@mredbishop
Copy link
Author

mredbishop commented May 17, 2016

If you're happy with that, we were planning on adding moment js to our fork of the project for exactly this reason! Should I crack on? :)

@herom
Copy link
Contributor

herom commented May 17, 2016

This would be awesome @mredbishop 👍 😸

@mredbishop
Copy link
Author

mredbishop commented May 17, 2016

Hey @herom, where do you test the xml parsing? I've included moment and written the code to support it but I'm not sure where you are testing the xml parser from?

@mredbishop
Copy link
Author

mredbishop commented May 17, 2016

Never mind I got it, I'll see if I can put it together and submit a pull request.

…rrideTimeZone and momentDateFormat on the options object when calling createClient(). Added tests to ensure it works. Added a vscode launch file for running and debugging the tests in vscode.
@coveralls
Copy link

coveralls commented May 18, 2016

Coverage Status

Coverage decreased (-0.003%) to 92.139% when pulling 29a08c8 on mredbishop:patch-1 into 2c9c537 on vpulim:master.

@mredbishop
Copy link
Author

mredbishop commented May 18, 2016

@herom I've added a test to cover this which is passing. Let me know if it needs anything else.

@@ -0,0 +1,4 @@
{
"momentDateTimeFormat": null,
"overrideTimeZone": "UTC"
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about just timezone?

@facundoolano
Copy link

would it make sense to merge the first commit while you work on the moment.js feature?

@jsdevel
Copy link
Collaborator

jsdevel commented Jul 21, 2016

we really need the API changes to be made first

@jsdevel
Copy link
Collaborator

jsdevel commented Apr 5, 2017

closing due to inactivity

@jsdevel jsdevel closed this Apr 5, 2017
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