Conversation
berkerpeksag
left a comment
berkerpeksag
left a comment
There was a problem hiding this comment.
Could you also rename 0006_auto_20171029_1524.py to a more readable name so we can easily see that it's a data migration?
| super().setUpClass() | ||
| cls.test_file_path = get_test_rss_path() | ||
| cls.entries = get_all_entries("file://{}".format(cls.test_file_path)) | ||
| cls.entries = get_all_entries("file:///{}".format(cls.test_file_path)) |
There was a problem hiding this comment.
Did you get a test failure without this change?
There was a problem hiding this comment.
I do indeed...
Traceback (most recent call last):
File "C:\Users\cianciustyles\Documents\Code\Python.org\blogs\tests\test_parser.py", line 17, in test_entries
self.assertEqual(len(self.entries), 25)
AssertionError: 0 != 25
but I can completely drop the first commit with the changes to make the tests pass on my host, if this already works for you.
| ] | ||
|
|
||
| operations = [ | ||
| migrations.RunPython(exclude_ending_day, include_ending_day) |
There was a problem hiding this comment.
Style nit: Please add trailing comma.
| def test_injest(self): | ||
| importer = ICSImporter(self.calendar) | ||
| with open(EVENTS_CALENDAR) as fh: | ||
| with open(EVENTS_CALENDAR, encoding='utf-8') as fh: |
There was a problem hiding this comment.
This shouldn't be necessary. What's the output of the following command in your system?
locale.getpreferredencoding(False)There was a problem hiding this comment.
On my Windows 10 laptop, the result is the following:
>>> import locale
>>> locale.getpreferredencoding(False)
'cp1252'
As in a previous comment, I think it would be better for the time being to drop completely these changes, and only commit the ones about the event importer with tests.
There was a problem hiding this comment.
I've never tried to run python.org tests under Windows so feel free to send those changes as a separate PR :)
| e.next_or_previous_time.dt_end | ||
| ) | ||
|
|
||
| ical = """\ |
There was a problem hiding this comment.
Can we move the new tests to separate test methods?
| make_aware(datetime(year=2013, month=8, day=2, hour=20, minute=30)), | ||
| e4.next_or_previous_time.dt_end | ||
| ) | ||
|
|
There was a problem hiding this comment.
Style nit: Please delete the extra line.
| """ | ||
| pep0_path = os.path.join(settings.PEP_REPO_PATH, 'pep-0000.html') | ||
| pep0_content = open(pep0_path).read() | ||
| pep0_content = open(pep0_path, encoding="utf-8 ").read() |
| from django.db import migrations | ||
|
|
||
|
|
||
| def exclude_ending_day(apps, _): |
There was a problem hiding this comment.
I'd replace _ with schema_editor and use it in:
...
db_alias = schema_editor.connection.alias
...
Model.objects.using(db_alias).filter(...)There was a problem hiding this comment.
I was not aware this was possible, I'm happy to change it.
|
|
||
| def exclude_ending_day(apps, _): | ||
| OccurringRule = apps.get_model('events', 'OccurringRule') | ||
| for occurring_rule in OccurringRule.objects.filter(all_day=True): |
There was a problem hiding this comment.
Can't we do this without a for loop?
from django.db.models import F
OccurringRule.objects.filter(all_day=True).update(dt_end=F('dt_end') - datetime.timedelta(days=1))
…y in the database
…which do not span for the whole day
CianciuStyles
force-pushed
the
fix-issue-375-2
branch
from
58dcf0e to
9c6c654
Compare
|
I think I have addressed all the changes you requested. If I forgot anything or some more changes are required, just let me know! |
|
Thanks! |
2 participants
Hello,
I'm sorry this took so long. I am opening a new pull request for closing issue #375, in which I addressed the approach we discussed in the other PR (#1161).
I have also added a test for issue #560, so hopefully this should fix that as well.