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 Parsing Issue #381

Closed
brownnrl opened this Issue Jan 15, 2014 · 22 comments

Comments

Projects
None yet
2 participants
@brownnrl
Contributor

brownnrl commented Jan 15, 2014

The following input causes a traceback when entering in a date:
'2 /15/2014'

Note the space after the month "2 ".

While technically an invalid input, this should probably be caught in validation rather than causing a scary traceback. It's easy enough to fudge your fingers moving between the month and day fields.

Application Name: moneyGuru
Version: 2.7.1

Traceback (most recent call last):
  File "qt\controller\panel.py", line 92, in widgetChanged
  File "core\gui\schedule_panel.py", line 64, in start_date
  File "core\app.py", line 169, in parse_date
  File "core\model\date.py", line 333, in parse_date
  File "core\model\date.py", line 376, in parse_date
  File "C:\Python\32-bit\3.3\lib\_strptime.py", line 500, in _strptime_datetime
  File "C:\Python\32-bit\3.3\lib\_strptime.py", line 337, in _strptime
ValueError: time data '2 /15/2014' does not match format '%m/%d/%Y'
@brownnrl

This comment has been minimized.

Contributor

brownnrl commented Jan 15, 2014

Note this is on a windows machine, I can test to see if it's repeatable on Ubuntu later.

Upon further investigation, you do validate the input but there is a set number of steps that causes the bug:

  • Open the window to create a new budget entry.
  • Go to the day field.
  • Left arrow to the month field.
  • Change the month number using a key (single digit).
  • Press the enter key.

You get the traceback.

@hsoft

This comment has been minimized.

Owner

hsoft commented Jan 16, 2014

moneyGuru does date validation, but this crash might be due to a mishandling of your date format. If you open your Preference panel (under the View menu), you'll see the date format that moneyGuru uses throughout the app. Could you tell me what it it please (it's not the same as in your traceback, that's another thing)?

@brownnrl

This comment has been minimized.

Contributor

brownnrl commented Jan 16, 2014

The preferences read 'M/d/yyyy' for the date format, which was unchanged or at least never directly set in the preferences panel. I was trying to track this down to an offending line, and I came up with the core.gui.date_widget module, lines 177-178 in the text property:

class DateWidget:
    #stuff [...]

    @property
    def text(self):
        #stuff [...]
        if self._buffer:
            elem2fmt[self._selected] = self._buffer.ljust(max(2, len(elem2fmt[self._selected])))

That adds that addtional space, I guess expecting the next key to possibly be the second digit. It's fine when you move off the selected month/day/year, but if you directly hit return while the digit character and the space is selected it gives that traceback.

@hsoft

This comment has been minimized.

Owner

hsoft commented Jan 16, 2014

That's interesting. Using your date format, I don't get the formatting crash all the time, but if I follow your instructions exactly, I can reproduce the crash.

Thanks, I'll fix this shortly.

BTW, the date format you have initially comes from your system preference, but you can override it if you want.

@brownnrl

This comment has been minimized.

Contributor

brownnrl commented Jan 16, 2014

I went through a couple of paths trying to think how I would do it, but not getting too far. My guess would be the above mentioned file, but I'll be very interested to see where you apply the fix. Thank you.

@hsoft

This comment has been minimized.

Owner

hsoft commented Jan 16, 2014

Yes, the problem is almost certainly in the DateWidget class.

If you're interested in getting your feet wet, a good place to start is probably to try to run the tests for this part of the code, which is in tests/gui/date_widget_test.py. It shouldn't be hard to write a test case that reproduce the bug, and from there, it might be easier to pinpoint the cause.

@brownnrl

This comment has been minimized.

Contributor

brownnrl commented Jan 17, 2014

I am out of time for today and have work tomorrow, but I have a good deal of time on the weekend to try to tackle it. If I have any problems I'll write back.

@brownnrl

This comment has been minimized.

Contributor

brownnrl commented Jan 18, 2014

After a preliminary investigation, I don't think that it can be solved within the date_widget module because the inclusion of the extra space during buffering is integral to the way that dates are input and it's not really possible to tell at the DateWidget level whether the current input is ready to be committed to the model.

The problem is that the focusOutEvent method is being processed after the widgetChanged event at the panel when the return key is pressed for whatever reason. Since the focusOutEvent eventually leads to the buffer being flushed and putting the input in a form that the model will consider valid, it needs to be processed first.

Considering this, I overloaded text() method of the support class (qt.support.date_edit module) to include a call to prepareDataForCommit:

    def text(self):
        self.prepareDataForCommit()
        return self.widget.text

This corrects the issue. However, I don't know what the implications for this in terms of flow of the application if are there any intermediate pulls for the widget text aside from the model. I am also not certain of cross platform changes that would be needed for Macs in their support classes.

I will work on this more later, please offer any advice or suggestions if I am off track.

@hsoft

This comment has been minimized.

Owner

hsoft commented Jan 18, 2014

Oh, the problem is deeper than I though then.

Overriding text() is probably not a good idea. After all, it's an information fetching method and is probably used as such by Qt (that is, called all the time). Making it mutate its content can have unforeseen consequences, like breaking the buffering system.

Maybe the problem lies in moneyGuru simply not using the appropriate events for what it does (either it shouldn't use focusOutEvent or it shouldn't use widgetChanged). It's been a while since I've coded this and maybe a revisit of these events is in order.

... but otherwise, I'd be inclined to fix this problem at the parse_date() level, that is, have it strip all spaces before parsing. After all, the only possible problem that we can have with this out-of-sync commit is to have an extra space somewhere.

@hsoft

This comment has been minimized.

Owner

hsoft commented Jan 18, 2014

Also, I can't reproduce the "The transaction is deleted" problem you mention. Does it happen with or without your text() override?

@brownnrl

This comment has been minimized.

Contributor

brownnrl commented Jan 18, 2014

I edited my last comment to remove that bit about transactions being deleted, I was just on an non-inclusive selected date range so it was just disappearing and not being deleted. Sorry. So that is not an issue.

I had filtered out the spaces in the model earlier, and it does fix the problem. But I was worried because technically you can set the separator character to be spaces in the preferences panel and it works for displaying and editing dates. If we go the filter-of-spaces route, maybe it would be advisable to put some small validation in the preferences panel with an automatic conversion of spaces to a '/' character. Just in case anyone is crazy enough to do that.

I could work on that and have it ready by the end of the day tomorrow.

@hsoft

This comment has been minimized.

Owner

hsoft commented Jan 19, 2014

No, moneyGuru doesn't let you use spaces as date separators, only /, - and .. If you try to put a space in the format, it will be removed as soon as you focus out of the field.

@brownnrl

This comment has been minimized.

Contributor

brownnrl commented Jan 19, 2014

If a space character is set in the preferences panel, and then you restart a space is the recognized separator. So I didn't know if it was a valid one.

The preferences don't seem to work with other characters besides the space and the ones you have mentioned above. So allowing the space character is probably a bug then.

moneyguru_space_in_dates

budgets_spaces_dates

@hsoft

This comment has been minimized.

Owner

hsoft commented Jan 19, 2014

Oh, wait a second. You are right. Spaces are allowed in date format (that was me yesterday commenting at ridiculous hours).

That does complicate things a little bit, but a solution at the parse_date() level is still possible:

  1. Verify if there's a space in the date format.
  2. If there's not, then it's safe to just strip all spaces from the date string.
  3. If there is, then what we have to do is to replace double spaces with single spaces.
@hsoft

This comment has been minimized.

Owner

hsoft commented Jan 19, 2014

I did a little bit of digging and that business with accepting spaces was added during the implementation of #325. I don't remember why and I don't document it, but I've explicitly added a test case for parsing dates with space separators, so I guess there was a good reason.

@brownnrl

This comment has been minimized.

Contributor

brownnrl commented Jan 19, 2014

The following seems to work for parse_date and corrects the issue. Unit tests do pass.

diff --git a/core/model/date.py b/core/model/date.py
index 0a2d2ee..bc58802 100644
--- a/core/model/date.py
+++ b/core/model/date.py
@@ -518,7 +518,13 @@ def parse_date(string, format):

     .. seealso:: :class:`DateFormat`
     """
-    return DateFormat(format).parse_date(string)
+    
+    format = DateFormat(format)
+
+    if format.separator == ' ':
+        return format.parse_date(string.replace('  ', ' '))
+
+    return format.parse_date(string.replace(' ', ''))

 def format_date(date, format):
     """Formats ``date`` using ``format`` (ISO).

So you replace double spaces with single space if the separator is a space, otherwise remove all spaces as you said.

@hsoft

This comment has been minimized.

Owner

hsoft commented Jan 19, 2014

I'm puzzled that all tests pass. It seems that if the date format has spaces in it, it will perform both replacements, and thus, in the end, remove all spaces.

Otherwise, I'd rather put that logic in DateFormat.parse_date(). That's the new place where we parse dates and the old parse_date() function is there for legacy purposes. Besides, I think many tests use DateFormat directly, which might explain why tests still pass.

@brownnrl

This comment has been minimized.

Contributor

brownnrl commented Jan 19, 2014

The conditional is if the date format's separator is a space, not that the format has a space in it. I moved the logic into the DateFormat.parse_date() function, all tests still pass as run from the core/tests/ directory.

diff --git a/core/model/date.py b/core/model/date.py
index 0a2d2ee..4b6ea61 100644
--- a/core/model/date.py
+++ b/core/model/date.py
@@ -580,6 +580,11 @@ class DateFormat:

     def parse_date(self, string):
         """Parses ``string`` to a ``datetime.date``."""
+        if self.separator == ' ':
+            string = string.replace('  ', ' ')
+        else:
+            string = string.replace(' ', '')
+
         return datetime.strptime(string, self.sys_format).date()

     def make_numerical(self):

Do you think that covers it? If it is, do I close the ticket or do you?

On another note, I'd like to contribute to other tickets if I'm able and you are interested in those contributions. Should I continue to paste patches as comments to those issues or should I fork and do pull requests through github? I'm not too familiar with github's pull request system, though I use git with redmine at work.

@hsoft

This comment has been minimized.

Owner

hsoft commented Jan 19, 2014

Looks good to me, thanks!

Since you've made the fix, you might as well get credits for it, so how about you submit a pull request with this patch?

However, the bug can't be considered fixed until two more things are added:

  1. A comment explaining why we do this, because it isn't self-evident. This comment can refer to this ticket.
  2. A test case for this bugfix.

I wouldn't mind doing it myself, but you might prefer having the satisfaction of fixing the whole issue yourself :)

Pull requests on github are nothing more than a branch that you push on your fork of moneyGuru and then ask the origin repo to include it. So, to make a pull request, all you have to do is to fix the bug locally, push your change on your fork, and from there, github will give you options to create pull requests for that branch.

Also: I suppose that you're working on the develop branch. But could you create your PR from the master branch? Since it's a bugfix, I commit it on master and merge into develop afterwards. This fix is going to result in a bugfix release, and that release isn't going to contain stuff currently in development.

@brownnrl

This comment has been minimized.

Contributor

brownnrl commented Jan 19, 2014

Ok, not an issue. I would like that very much if I could add the comment, test case, get the credit, etc. I can't do so now, the wife wants to go and be social. But when I get back later tonight or tomorrow morning, I will finish up the work required to close the ticket per your comment. Thank you!

brownnrl added a commit to brownnrl/moneyguru that referenced this issue Jan 20, 2014

@hsoft hsoft closed this in 1273bb2 Jan 20, 2014

@hsoft

This comment has been minimized.

Owner

hsoft commented Jan 24, 2014

@brownnrl do I add you to the credits? I use your full name from your youtube account?

@brownnrl

This comment has been minimized.

Contributor

brownnrl commented Jan 24, 2014

I don't know. This is my first open source project. Ahh... sure. You can
use my full name, Nelson Brown. Thank you.

On Fri, Jan 24, 2014 at 3:55 PM, Virgil Dupras notifications@github.comwrote:

@brownnrl https://github.com/brownnrl do I add you to the credits? I
use your full name from your youtube account?


Reply to this email directly or view it on GitHubhttps://github.com//issues/381#issuecomment-33259880
.

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