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

Fix #2653 - Make DATE_PATH follow rrule #2656

Merged
merged 6 commits into from Feb 6, 2017
Merged

Conversation

@sukiletxe
Copy link
Contributor

sukiletxe commented Jan 31, 2017

No description provided.

@sukiletxe
Copy link
Contributor Author

sukiletxe commented Jan 31, 2017

I don't know why Travis fails only in Python 3.5. Although Codacy fails, that part of the code was in the original. Should I fix it?

@felixfontein
Copy link
Contributor

felixfontein commented Jan 31, 2017

You can ignore the travis failure, it looks like a random fluke / test-related problem which has nothing to do with your changes.

Regarding the codacy issue: no, it wasn't there before your commit; before, it was calling datetime.datetime.now with that argument, and datetime.datetime.now is a callable function, while the result of datetime.datetime.strptime is not.

@felixfontein
Copy link
Contributor

felixfontein commented Jan 31, 2017

Your changes don't work for me:

$ nikola new_post -d
Creating New Post
-----------------

Title: test
Scanning posts......done!
Traceback (most recent call last):
  File "~/doit/doit/doit_cmd.py", line 167, in run
    return command.parse_execute(args)
  File "~/doit/doit/cmd_base.py", line 120, in parse_execute
    return self.execute(params, args)
  File "~/nikola-original/nikola/plugin_categories.py", line 130, in execute
    return self._execute(options, args)
  File "~/nikola-original/nikola/plugins/command/new_post.py", line 344, in _execute
    dateobj = datetime.datetime.strptime(data['date'].split('+')[0], '%Y-%m-%d %H:%M:%S')
  File "/usr/lib/python3.6/_strptime.py", line 565, in _strptime_datetime
    tt, fraction = _strptime(data_string, format)
  File "/usr/lib/python3.6/_strptime.py", line 365, in _strptime
    data_string[found.end():])
ValueError: unconverted data remains:  UTC

If I undo the change, I get:

$ nikola new_post -d
Creating New Post
-----------------

Title: test
Scanning posts......done!
[2017-01-31T19:53:24Z] INFO: new_post: Your post's text is at: posts/2017/01/31/test.rst

(as expected)

@felixfontein
Copy link
Contributor

felixfontein commented Jan 31, 2017

Printing data['date'] yields 2017-01-31 19:54:51 UTC, which explains why it doesn't work here.

You should modify get_date() (line 65 in the same file) to also return the date stamp itself, and then work with it instead of re-parsing the string returned by get_date().

@sukiletxe
Copy link
Contributor Author

sukiletxe commented Feb 1, 2017

Done. Thanks @felixfontein for your comments.

@felixfontein
Copy link
Contributor

felixfontein commented Feb 1, 2017

The failing tests are completely unrelated to this PR. @Kwpolska: it looks like the baseline is out of date (a pygments update or so?). Also, there's a problem with Codacy coverage reporting:

$ if [[ $NMODE == 'nikola' ]]; then scripts/codacity_coverage.sh; fi
2017-02-01 17:08:30,876 - ERROR - environment variable CODACY_PROJECT_TOKEN is not defined.
@felixfontein
Copy link
Contributor

felixfontein commented Feb 1, 2017

How about replacing the tests in tests/test_scheduling.py by the following? Then the full return value of get_date() will be tested. (Disclaimer: I didn't test whether this actually works. I just did some replacements in a text editor.)

        # NOW does not match rule #########################################
        # No last date
        expected = TODAY.replace(day=23)
        self.assertEqual((expected, expected.strftime(FMT)), get_date(True, RULE_FR, tz=UTC))
        self.assertEqual((expected, expected.strftime(FMT)), get_date(True, RULE_FR, tz=UTC))

        # Last date in the past; doesn't match rule
        date = TODAY.replace(hour=7)
        expected = TODAY.replace(day=23, hour=7)
        self.assertEqual((expected, expected.strftime(FMT)), get_date(True, RULE_FR, date, tz=UTC))

        # Last date in the future; doesn't match rule
        date = TODAY.replace(day=24, hour=7)
        expected = TODAY.replace(day=30, hour=7)
        self.assertEqual((expected, expected.strftime(FMT)), get_date(True, RULE_FR, date, tz=UTC))

        # Last date in the past; matches rule
        date = TODAY.replace(day=16, hour=8)
        expected = TODAY.replace(day=23, hour=8)
        self.assertEqual((expected, expected.strftime(FMT)), get_date(True, RULE_FR, date, tz=UTC))

        # Last date in the future; matches rule
        date = TODAY.replace(day=23, hour=18)
        expected = TODAY.replace(day=30, hour=18)
        self.assertEqual((expected, expected.strftime(FMT)), get_date(True, RULE_FR, date, tz=UTC))

        # NOW matches rule ################################################
        # Not scheduling should return NOW
        self.assertEqual((NOW, _NOW), get_date(False, RULE_TH, tz=UTC))
        # No last date
        self.assertEqual((NOW, _NOW), get_date(True, RULE_TH, tz=UTC))
        self.assertEqual((NOW, _NOW), get_date(True, RULE_TH, tz=UTC))

        # Last date in the past; doesn't match rule
        # Corresponding time has already passed, today
        date = TODAY.replace(day=21, hour=7)
        expected = TODAY.replace(day=29, hour=7)
        self.assertEqual((expected, expected.strftime(FMT)), get_date(True, RULE_TH, date, tz=UTC))
        # Corresponding time has not passed today
        date = TODAY.replace(day=21, hour=18)
        expected = TODAY.replace(day=22, hour=18)
        self.assertEqual((expected, expected.strftime(FMT)), get_date(True, RULE_TH, date, tz=UTC))

        # Last date in the future; doesn't match rule
        # Corresponding time has already passed, today
        date = TODAY.replace(day=24, hour=7)
        expected = TODAY.replace(day=29, hour=7)
        self.assertEqual((expected, expected.strftime(FMT)), get_date(True, RULE_TH, date, tz=UTC))
        # Corresponding time has not passed today
        date = TODAY.replace(day=24, hour=18)
        expected = TODAY.replace(day=29, hour=18)
        self.assertEqual((expected, expected.strftime(FMT)), get_date(True, RULE_TH, date, tz=UTC))

        # Last date in the past; matches rule
        # Corresponding time has already passed, today
        date = TODAY.replace(day=15, hour=7)
        expected = TODAY.replace(day=29, hour=7)
        self.assertEqual((expected, expected.strftime(FMT)), get_date(True, RULE_TH, date, tz=UTC))
        # Corresponding time has already passed, today; rule specifies HOUR
        date = TODAY.replace(day=15, hour=7)
        expected = TODAY.replace(day=29, hour=9)
        self.assertEqual((expected, expected.strftime(FMT)), get_date(True, RULE_TH + ';BYHOUR=9', date, tz=UTC))
        # Corresponding time has not passed today
        date = TODAY.replace(day=15, hour=18)
        expected = TODAY.replace(day=22, hour=18)
        self.assertEqual((expected, expected.strftime(FMT)), get_date(True, RULE_TH, date, tz=UTC))

        # Last date in the future; matches rule
        # Corresponding time has already passed, today
        date = TODAY.replace(day=29, hour=7)
        expected = TODAY.replace(day=5, month=9, hour=7)
        self.assertEqual((expected, expected.strftime(FMT)), get_date(True, RULE_TH, date, tz=UTC))
        # Corresponding time has not passed today
        date = TODAY.replace(day=22, hour=18)
        expected = TODAY.replace(day=29, hour=18)
        self.assertEqual((expected, expected.strftime(FMT)), get_date(True, RULE_TH, date, tz=UTC))
@felixfontein
Copy link
Contributor

felixfontein commented Feb 2, 2017

In the code I posted above, replace (expected, expected.strftime(FMT)) by (expected.strftime(FMT), expected); then it works.

The .replace('+00:00', ' UTC') is not necessary, and (NOW, _NOW) was correct.

@felixfontein
Copy link
Contributor

felixfontein commented Feb 2, 2017

Also, you can add yourself to AUTHORS.txt while you're at it.

@felixfontein
Copy link
Contributor

felixfontein commented Feb 2, 2017

Ah. Already there, then :)

@sukiletxe
Copy link
Contributor Author

sukiletxe commented Feb 2, 2017

OK, I've screwed things up in a completely stupid way. Let me fix this. I'm really sorry for the trouble.

@felixfontein
Copy link
Contributor

felixfontein commented Feb 3, 2017

No problem! Take your time.

@sukiletxe
Copy link
Contributor Author

sukiletxe commented Feb 3, 2017

OK, this is done at last. I'm sorry again for the trouble. By the way, there are two tests in test_scheduling.py (the first and the second), which I believe test the same thing (this is also in master).

@sukiletxe
Copy link
Contributor Author

sukiletxe commented Feb 3, 2017

@felixfontein: Actually, if I understood the code correctly, the replacements are in fact necessary, as get_date returns UTC if the timezone is UTC, whereas %z returns +00:00.

@felixfontein
Copy link
Contributor

felixfontein commented Feb 3, 2017

But it isn't using %z, but %Z, which is supposed to return the name of the timezone (see https://docs.python.org/3/library/datetime.html#strftime-and-strptime-behavior; the examples explicitly mentions UTC). So the replacement should be unnecessary.

Which Python version under which operating system do you have which returns +00:00 instead of UTC for %Z?

@sukiletxe
Copy link
Contributor Author

sukiletxe commented Feb 4, 2017

@felixfontein
Copy link
Contributor

felixfontein commented Feb 4, 2017

Well, so far it worked fine without the replace, both on travis and on my local machine (and on other machines as well). So it should be unnecessary.

@Kwpolska: what do you think? Are these replacements OK?

@Kwpolska
Copy link
Member

Kwpolska commented Feb 4, 2017

These don’t match because two different methods are in use. new_post uses some special (more human) rules to produce those strings, while the test suite is using %Z. But do we really need to test those strings? Why not just test equality of datetime objects, now that get_date() also returns them?

@sukiletxe
Copy link
Contributor Author

sukiletxe commented Feb 6, 2017

Now that I think about it, the replacements weren't necessary in the first tests update (I rebased too much and that diff is gone now, sorry).
Maybe testing equality could be a good idea (that is, equality between expected and get_date()[1]), as datetime.datetime.strftime is indeed predictable, although I don't see anything incorrect in the current behaviour. So as you wish, agree among yourselves and I'll do it (no rebases, I promise).

@Kwpolska
Copy link
Member

Kwpolska commented Feb 6, 2017

Just change those tests to look only at the datetime object.

@sukiletxe
Copy link
Contributor Author

sukiletxe commented Feb 6, 2017

@Kwpolska
Copy link
Member

Kwpolska commented Feb 6, 2017

Travis tests fail because you are not a member of the @getnikola organization, and thus Travis prevents your PR from accessing the token (which is pretty smart from the security perspective). I fixed that on master for future PRs, and I’m merging yours.

Thank you for contributing to Nikola! 🎉

@Kwpolska Kwpolska merged commit 95d869d into getnikola:master Feb 6, 2017
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
codacy/pr Good work! A positive pull request.
Details
@sukiletxe
Copy link
Contributor Author

sukiletxe commented Feb 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.