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

Add parseDate option #273

Closed
wants to merge 2 commits into from
Closed

Conversation

dsl101
Copy link

@dsl101 dsl101 commented Nov 13, 2018

Finally after only 2 years :). I needed this on another project, and eventually got round to making a PR.

Finally, after only two years! I got back to the project I was working on, and needed this functionality, so made a PR against the latest code. Assume nothing else has changed which might affect it, but happy to
Fixed one left-over option name, and switched to ISO string, so easier to handle internationally. Probably.
Copy link
Owner

@danmactough danmactough left a comment

Choose a reason for hiding this comment

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

Thanks for your pull request @dsl101. Let me know if you have any questions about my comments.

@@ -71,6 +71,7 @@ function FeedParser (options) {
if (!('strict' in this.options)) this.options.strict = false;
if (!('normalize' in this.options)) this.options.normalize = true;
if (!('addmeta' in this.options)) this.options.addmeta = true;
if (!('parseDate' in this.options)) this.options.parseDate = true;
Copy link
Owner

Choose a reason for hiding this comment

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

We've already parsed the date string into a Date object. With this option enabled, we'll actually be stringifying the date. So, let's rename this option: maybe stringifydate. (If you have a different suggestion, that's ok, but let's also ensure the option names are consistent -- one word, all lowercase.) (I know, I know, there's resume_saxerror which has an underscore -- 🤷‍♂️ )

Also, we need to preserve the current behavior as the default, so if the option is not provided, it's value should be false.

@@ -430,6 +431,7 @@ FeedParser.prototype.handleMeta = function handleMeta (node, type, options) {

var meta = {}
, normalize = !options || (options && options.normalize)
, parseDate = !options || (options && options.parseDate)
Copy link
Owner

Choose a reason for hiding this comment

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

parseDate = !options || (options && options.parseDate)

This logic will need to be flipped: stringifydate should be true only if options && options.stringifydate is true.

@@ -777,6 +780,7 @@ FeedParser.prototype.handleItem = function handleItem (node, type, options){

var item = {}
, normalize = !options || (options && options.normalize)
, parseDate = !options || (options && options.parseDate)
Copy link
Owner

Choose a reason for hiding this comment

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

Same comment as above.

@dsl101
Copy link
Author

dsl101 commented Nov 13, 2018

Yes, that's fine. I was going by what you'd originally said here :). Maybe I misunderstood what you meant? Anyway, I'm away for a few days now but will edit when I'm back.

My reason for doing this is all firebase related. When used in a callable firebase cloud function, the date objects in the response from feedparser are all returned to my client-side app as empty objects for some reason. I suspect maybe a bug in firebase, but to work round I have to convert all dates to strings before the function returns. ISO date will work for me, but to make it more flexible, it could be an option called dateformat which, if omitted, leaves it as a date object, but could have values like toISOString, 'toLocaleString`, etc. to match the native string formats that the Date object can convert to. What do you think?

@danmactough
Copy link
Owner

Yes, that's fine. I was going by what you'd originally said here :). Maybe I misunderstood what you meant?

Nope. You understood fine. I guess I'm ... mercurial. 😂

@dsl101
Copy link
Author

dsl101 commented Nov 16, 2018

This is superseded by the other PR. Closing this one.

@dsl101 dsl101 closed this Nov 16, 2018
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.

2 participants