bug fix and possible fix for #11 #18

JBKahn · 2014-12-03T02:50:19Z

The use of lists there seems to make sense (should be forced as a list, right?).

I'm not sure why you're specifically removing the timezone?

This resolves #11 if that was simply a bug. Couldn't tell the intent of the line.

In [1]: from dateparser.date import DateDataParser

In [2]: ddp = DateDataParser()

In [3]: ddp.get_date_data('2014-10-09T17:57:39+00:00')['date_obj']
Out[3]: datetime.datetime(2014, 10, 9, 17, 57, 39, tzinfo=tzutc())

In [4]: ddp.get_date_data('2014-10-09T17:57:39+00:00', '')['date_obj']
Out[4]: datetime.datetime(2014, 10, 9, 17, 57, 39, tzinfo=tzutc())

In [5]: ddp.get_date_data('2014-10-09T17:57:39+00:00', '%Y')['date_obj']
Out[5]: datetime.datetime(2014, 10, 9, 17, 57, 39, tzinfo=tzutc())

JBKahn · 2014-12-03T02:59:39Z

----------------------------------------------------------------------
Ran 106 tests in 0.173s

OK

and no linting errors.

eliasdorneles · 2014-12-04T11:59:22Z

Hmmm, the thing is, for our internal project that used this code this was actually a feature, we wanted the timezone information out (it's a bit ugly, I know).

But I agree this needs rethinking, and I'm willing to accept your proposal -- we just need to check our internal code first. Gimme a few days and I'll get back to this.

Also, it will be great to have tests that make these expectations explicit. =)

JBKahn · 2014-12-04T18:03:08Z

Ok, I'll add some tests. But I'll be lazy about it! Possibly. Or I'll have
them done later today.

On Thu, Dec 4, 2014, 6:59 AM Elias Dorneles notifications@github.com
wrote:

Hmmm, the thing is, for our internal project that used this code this was
actually a feature, we wanted the timezone information out (it's a bit
ugly, I know).

But I agree this needs rethinking, and I'm willing to accept your proposal
-- we just need to check our internal code first. Gimme a few days and I'll
get back to this.

Also, it will be great to have tests that make these expectations
explicit. =)

—
Reply to this email directly or view it on GitHub
#18 (comment).

Allactaga · 2015-01-05T15:30:29Z

Hi, @JBKahn. Happy New Year! 😃
Are you still planning to work further on this request or can we assign someone to it?

JBKahn · 2015-01-05T15:57:23Z

Yup, I wasn't on my computer a lot over the holidays. I'll pick this up over the next few days

Allactaga · 2015-01-05T15:57:58Z

Great, thanks! 😃

…t-arg-bug

JBKahn · 2015-01-17T20:05:42Z

It's been a long time since I've looked over this. Is that the test you were looking for here?

----------------------------------------------------------------------
Ran 113 tests in 2.364s

Allactaga · 2015-02-09T10:28:11Z

Hi @JBKahn , sorry for taking me so long to respond. Can you please merge master branch in yours? We made some major changes there.

JBKahn · 2015-02-09T15:01:52Z

sure

…to date-format-arg-bug Conflicts: dateparser/date.py tests/test_date.py

JBKahn · 2015-02-09T15:07:57Z

I'll take another look when I have time, I'm just going to assume the tests won't pass and I'll need to fix something.

JBKahn · 2015-02-09T15:12:55Z

well I'll be damned. I'm better at this merging business than I thought.

Allactaga · 2015-02-09T18:15:17Z

dateparser/date.py

@@ -175,7 +175,7 @@ def _try_given_formats(self):
        if not self.date_formats:
            return

-        return parse_with_formats(self._get_translated_date_with_formatting(), self.date_formats)
+        return parse_with_formats(self._get_translated_date_with_formatting(), list(self.date_formats))


Could you please remind me why do we need to construct list here?

It's not garunteed to be an iterable and in the event that it isn't it used
to to something foolish in the end as well as iterate over the string.

On Mon, Feb 9, 2015, 1:15 PM Eugene Amirov notifications@github.com wrote:

In dateparser/date.py
#18 (comment):

@@ -175,7 +175,7 @@ def _try_given_formats(self):
if not self.date_formats:
return

return parse_with_formats(self._get_translated_date_with_formatting(), self.date_formats)

return parse_with_formats(self._get_translated_date_with_formatting(), list(self.date_formats))

Could you please remind me why do we need to construct list here?

—
Reply to this email directly or view it on GitHub
https://github.com/scrapinghub/dateparser/pull/18/files#r24350439.

But how would this change help in such case?

>>> list("qwerty") ['q', 'w', 'e', 'r', 't', 'y']

Right, I never got around to running a test, should have instead put it in
a list if it's a string. Haven't looked at this in a while

On Mon, Feb 9, 2015, 5:39 PM Eugene Amirov notifications@github.com wrote:

In dateparser/date.py
#18 (comment):

@@ -175,7 +175,7 @@ def _try_given_formats(self):
if not self.date_formats:
return

return parse_with_formats(self._get_translated_date_with_formatting(), self.date_formats)

return parse_with_formats(self._get_translated_date_with_formatting(), list(self.date_formats))

But how would this change help in such case?

list("qwerty")
['q', 'w', 'e', 'r', 't', 'y']

—
Reply to this email directly or view it on GitHub
https://github.com/scrapinghub/dateparser/pull/18/files#r24372801.

We used this check for similar case. May be it is worth to add this group to dateparser.utils as plain_iterable or something. What do you think?

I could get behind that check, works for me. When I get back on my laptop
I'll make the change and run the tests

On Mon, Feb 9, 2015, 5:44 PM Eugene Amirov notifications@github.com wrote:

In dateparser/date.py
#18 (comment):

@@ -175,7 +175,7 @@ def _try_given_formats(self):
if not self.date_formats:
return

return parse_with_formats(self._get_translated_date_with_formatting(), self.date_formats)

return parse_with_formats(self._get_translated_date_with_formatting(), list(self.date_formats))

We used this check
https://github.com/scrapinghub/dateparser/blob/master/dateparser/date.py#L222
for similar case. May be it is worth to add this group to dateparser.utils
as plain_iterable or something. What do you think?

—
Reply to this email directly or view it on GitHub
https://github.com/scrapinghub/dateparser/pull/18/files#r24373150.

Allactaga · 2015-02-18T16:56:34Z

Hi @JBKahn, how are you?
What do you say if we finish with this until someone landed more merge conflicts in master? :)

JBKahn · 2015-02-18T17:27:30Z

I'll do it tonight.

On Wed, Feb 18, 2015, 11:56 AM Eugene Amirov notifications@github.com
wrote:

Hi @JBKahn https://github.com/JBKahn, how are you?
What do you say if we finish with this until someone landed more merge
conflicts in master? :)

—
Reply to this email directly or view it on GitHub
#18 (comment).

…to date-format-arg-bug

JBKahn · 2015-02-19T02:33:45Z

Should be good?

Allactaga · 2015-02-19T08:22:23Z

Good for me, but what do you think: wouldn't it be better to raise an error there? I mean so that someone wouldn't pass a dict or object.

JBKahn · 2015-02-19T12:06:33Z

Sure, that works for me. So long as it's handled gracefully.

On Thu, Feb 19, 2015, 3:22 AM Eugene Amirov notifications@github.com
wrote:

Good for me, but what do you think: wouldn't it be better to raise an
error there? I mean so that someone wouldn't pass a dict or object.

—
Reply to this email directly or view it on GitHub
#18 (comment).

Allactaga · 2015-02-19T12:24:03Z

Will you make proper change? Or if you want to, i can make it and merge the branch.

JBKahn · 2015-02-19T13:19:34Z

If you could make the change, that would be great!

On Thu, Feb 19, 2015, 7:24 AM Eugene Amirov notifications@github.com
wrote:

Will you make proper change? Or if you want to, i can make it and merge
the branch.

—
Reply to this email directly or view it on GitHub
#18 (comment).

Allactaga · 2015-02-19T14:18:20Z

Sure.

bug fix and possible fix for #11

Allactaga · 2015-02-19T14:36:14Z

Here are the changes i made.

bug fix and possible fix

5fdb217

JBKahn changed the title ~~bug fix and possible fix~~ bug fix and possible fix for #11 Dec 3, 2014

accidentally got rid of a line

2066015

JBKahn added 2 commits January 17, 2015 14:57

Merge branch 'master' of github.com:JBKahn/dateparser into date-forma…

8809e3b

…t-arg-bug

adding a test

723f96b

Merge branch 'master' of https://github.com/scrapinghub/dateparser in…

7c7eb36

…to date-format-arg-bug Conflicts: dateparser/date.py tests/test_date.py

Allactaga reviewed Feb 9, 2015
View reviewed changes

JBKahn added 2 commits February 18, 2015 21:28

Merge branch 'master' of https://github.com/scrapinghub/dateparser in…

c6fd92a

…to date-format-arg-bug

fixing list check

7f60e01

Allactaga added a commit that referenced this pull request Feb 19, 2015

Merge pull request #18 from JBKahn/date-format-arg-bug

2e28df5

bug fix and possible fix for #11

Allactaga merged commit 2e28df5 into scrapinghub:master Feb 19, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bug fix and possible fix for #11 #18

bug fix and possible fix for #11 #18

JBKahn commented Dec 3, 2014

JBKahn commented Dec 3, 2014

eliasdorneles commented Dec 4, 2014

JBKahn commented Dec 4, 2014

Allactaga commented Jan 5, 2015

JBKahn commented Jan 5, 2015

Allactaga commented Jan 5, 2015

JBKahn commented Jan 17, 2015

Allactaga commented Feb 9, 2015

JBKahn commented Feb 9, 2015

JBKahn commented Feb 9, 2015

JBKahn commented Feb 9, 2015

Allactaga Feb 9, 2015

JBKahn Feb 9, 2015

Allactaga Feb 9, 2015

JBKahn Feb 9, 2015

Allactaga Feb 9, 2015

JBKahn Feb 9, 2015

Allactaga commented Feb 18, 2015

JBKahn commented Feb 18, 2015

JBKahn commented Feb 19, 2015

Allactaga commented Feb 19, 2015

JBKahn commented Feb 19, 2015

Allactaga commented Feb 19, 2015

JBKahn commented Feb 19, 2015

Allactaga commented Feb 19, 2015

Allactaga commented Feb 19, 2015

bug fix and possible fix for #11 #18

bug fix and possible fix for #11 #18

Conversation

JBKahn commented Dec 3, 2014

JBKahn commented Dec 3, 2014

eliasdorneles commented Dec 4, 2014

JBKahn commented Dec 4, 2014

Allactaga commented Jan 5, 2015

JBKahn commented Jan 5, 2015

Allactaga commented Jan 5, 2015

JBKahn commented Jan 17, 2015

Allactaga commented Feb 9, 2015

JBKahn commented Feb 9, 2015

JBKahn commented Feb 9, 2015

JBKahn commented Feb 9, 2015

Allactaga Feb 9, 2015

Choose a reason for hiding this comment

JBKahn Feb 9, 2015

Choose a reason for hiding this comment

Allactaga Feb 9, 2015

Choose a reason for hiding this comment

JBKahn Feb 9, 2015

Choose a reason for hiding this comment

Allactaga Feb 9, 2015

Choose a reason for hiding this comment

JBKahn Feb 9, 2015

Choose a reason for hiding this comment

Allactaga commented Feb 18, 2015

JBKahn commented Feb 18, 2015

JBKahn commented Feb 19, 2015

Allactaga commented Feb 19, 2015

JBKahn commented Feb 19, 2015

Allactaga commented Feb 19, 2015

JBKahn commented Feb 19, 2015

Allactaga commented Feb 19, 2015

Allactaga commented Feb 19, 2015