Parse and store Event dates

frantisekz commented

2020-11-19 08:59:54 +00:00

Contributor

This will be split into multiple Pull Requests.

In the first one, the aim is to add functions to parse testday date ranges from wiki, update db models, wire the parsing to testday creation (not tested yet) and wire up code to update past testdays (not tested yet) .

And finally, I'll change the UI (with other tweaks) to distinguish active and past testdays.

This will be split into multiple Pull Requests. In the first one, the aim is to add functions to parse testday date ranges from wiki, update db models, wire the parsing to testday creation (not tested yet) and wire up code to update past testdays (not tested yet) . And finally, I'll change the UI (with other tweaks) to distinguish active and past testdays.

frantisekz commented

2020-11-19 09:12:17 +00:00

Author

Contributor

rebased onto 8c2c42d606d783aaf85a044d99747ae315163e9d

frantisekz commented

2020-11-19 10:47:25 +00:00

Author

Contributor

1 new commit added

Process past events

**1 new commit added** * ``Process past events``

frantisekz commented

2020-11-19 10:48:40 +00:00

Author

Contributor

2 new commits added

Process past events
Parse and store Event dates

**2 new commits added** * ``Process past events`` * ``Parse and store Event dates``

jskladan commented

2020-11-19 10:58:19 +00:00

Contributor

I don't see how this could work, if you are on the 1e0fb82777af revision, the Event table does not have the start/end columns yet.

If you want to retrospectively fill the dates, do it as a part of the DB schema upgrade.

I don't see how this could work, if you are on the `1e0fb82777af` revision, the `Event` table does not have the `start/end` columns yet. If you want to retrospectively fill the dates, do it as a part of the DB schema upgrade.

jskladan commented

2020-11-19 11:02:15 +00:00

Contributor

Ah, nevermind, the current_rev is filled before the upgrade... I'd still rather see the misc.testdays_process_dates() call in the migration script, though

Ah, nevermind, the `current_rev` is filled *before* the upgrade... I'd still rather see the `misc.testdays_process_dates()` call in the migration script, though

jskladan commented

2020-11-19 11:04:02 +00:00

Contributor

Since you are using the method out of the wiki.py scope, please remove the __ prefix.

Since you are using the method out of the `wiki.py` scope, please remove the `__` prefix.

jskladan commented

2020-11-19 11:04:57 +00:00

Contributor

How about start, end = ... instead? May not be the best option, given how you use the data later on, just pointing to the option of "better readability".

How about `start, end = ...` instead? May not be the best option, given how you use the data later on, just pointing to the option of "better readability".

jskladan commented

2020-11-19 11:06:14 +00:00

Contributor

Also, since the testdays_process_dates() is only used here (to actually "fill in the missing dates"), consider renaming it to something more indicative of what it does.

Also, since the `testdays_process_dates()` is only used here (to actually "fill in the missing dates"), consider renaming it to something more indicative of what it does.

jskladan commented

2020-11-19 11:10:28 +00:00

Contributor

As I already mentioned elsewhere, since this is only really ever used to "fill in the data for old testdays", consider moving this code into the migration script instead. Would make way more sense than the current solution.

jskladan commented

2020-11-19 11:13:28 +00:00

Contributor

In general, rather than returing None, consider raising a custom exception instead. That IMO makes more sense in the context of what the code does, and the code "working around" the "it either returns None or a tuple with the dates" concept would also get nicer.

In general, rather than returing `None`, consider raising a custom exception instead. That IMO makes more sense in the context of what the code does, and the code "working around" the "it either returns `None` or a tuple with the dates" concept would also get nicer.

jskladan commented

2020-11-19 11:17:24 +00:00

Contributor

I don't really like this. I understand the "saving characters" motivation, but adding an arbitrary attribute to the class is not the best solution to the problem at hand.

I'd rather see something in the line of:

def __init__(self, name, metadata_url, testday_url=None, testday_start=None, testday_end=None, resultsdb_job_uuid=None):
    self.testday_start = testday_start or datetime.datetime(1900, 1, 1, 0, 0)
    self.testday_end = testday_end or datetime.datetime(1900, 1, 1, 0, 0)

I don't really like this. I understand the "saving characters" motivation, but adding an arbitrary attribute to the class is not the best solution to the problem at hand. I'd rather see something in the line of: ``` def __init__(self, name, metadata_url, testday_url=None, testday_start=None, testday_end=None, resultsdb_job_uuid=None): self.testday_start = testday_start or datetime.datetime(1900, 1, 1, 0, 0) self.testday_end = testday_end or datetime.datetime(1900, 1, 1, 0, 0) ```

jskladan commented

2020-11-19 11:21:19 +00:00

Contributor

Not a huge fan of the blanket except statement. Ideally just catch what is necessary, if that's not possible, then explain the reasoning in a comment, and at least log the exception, so one could investigate.