Add course info cards & calendars for runs #259
Conversation
The course page was the only thing that inherited from _lessons_list
Helps with: pyvec#255
|
This looks good by the first quick look. BTW what linter do you use? |
|
A homegrown hack, see pyvec/elsa#33 |
| return ''.join(parts).format(start=start, end=end) | ||
|
|
||
| @template_filter() | ||
| def monthname(number): |
There was a problem hiding this comment.
Any particular reason not to use the standard library? I don't mind hardocding this here, since it's not very likely the month names will change any time soon. However using LC_TIME will make easier to switch the entire website to let's say Slovak, if we ever want to do that.
>>> import locale
>>> import calendar
>>> locale.setlocale(locale.LC_TIME, 'cs_CZ.utf-8')
'cs_CZ.utf-8'
>>> calendar.month_name[3].title()
'Březen'There was a problem hiding this comment.
That's a reasonable thing to say. But I don't trust locales.
In practice I found locales frequently solve either too much or too litle. See for example naucse's function for formatting date ranges – building that on top of a locale "format a date for me" function would probably turn out quite fragile, so anyway you end up with some things based on locale and some hard-coded, which means some explicit and some implicit, and you anyway need to take each language into account separately, and... it's just a mess.
Also, it's one more dependency: cs_CZ needs to be available on the target system. How do I ensure that? How do I test it? And, setlocale is not thread-safe. Using it would rule out the possibility of running the Czech and Slovak version on one (threaded) server.
The flip side is that the site can only be translated by programmers (or a team that has a programmer). And I am fine with that, at least for now.
There was a problem hiding this comment.
Makes sense. As I said, I was just curious.
naucse/routes.py
Outdated
| months = [] | ||
| year = course.start_date.year | ||
| month = course.start_date.month | ||
| while (year, month) <= (course.end_date.year, course.end_date.month): |
There was a problem hiding this comment.
I am wondering, if there possibly isn't already a function, that does this?
>>> tuple((d.year, d.month) for d in dateutil.rrule.rrule(dateutil.rrule.MONTHLY, dtstart=datetime.date(year=2017, month=1, day=1), until=datetime.date(year=2018, month=2, day=1)))
((2017, 1), (2017, 2), (2017, 3), (2017, 4), (2017, 5), (2017, 6), (2017, 7), (2017, 8), (2017, 9), (2017, 10), (2017, 11), (2017, 12), (2018, 1), (2018, 2))Now I don't know if this would be any better, faster or more readable (probably not). However when dealing with dates, I'd probably prefer to stick with the date type. (I know, those are in fact months...)
Anyway: please make it a function (regardless of the implementation). That would make it:
- testable
- easily refactored if needed
- more readable for a reader of this route (if you pick a descriptive name, maybe
months_range(start_date, end_date))
There was a problem hiding this comment.
I think (year, month) tuples are a natural representation here. Datetime objects don't offer much functionality for using them as months.
I went with list_months; I didn't use “range” as the end date is included.
naucse/routes.py
Outdated
| @@ -117,13 +117,35 @@ def course(course): | |||
| def lesson_url(lesson, *args, **kwargs): | |||
| return url_for('course_page', course=course, lesson=lesson, *args, **kwargs) | |||
|
|
|||
| recent_runs = [] | |||
| if not course.start_date: | |||
There was a problem hiding this comment.
I'd make this a function/method/whatnot as well (see the next comment that I made earlier). It makes the route (its code) too long.
| </p> | ||
| <ul> | ||
| <li> | ||
| na <a href="https://www.facebook.com/groups/pyonieri/">Facebooku</a>, |
There was a problem hiding this comment.
I don't like having (what I think of as) content in templates. However, for the lack of better possibilities, let's keep it that way for now.
|
Good points; I'll address them as I find time. |
- Move the card below text on "medium" screens -- it was too cramped there - When the card is below text, add a margin before it
This makes it - testable - easily refactored if needed - more readable for a reader of this route
|
Don't know if you'd like to squash some commits or not, so I leave merging to you. |
|
Oh, I've introduced a conflict. Shall I rebase, or would you like to do that? |
|
It's a trivial conflict, I'll merge. |
|
Oh, GitHub just did something crazy. |
iCal export is still missing.
There are also a few unrelated commits – link style fix and Bootstrap update. I'll pull them out into separate PRs if the review is long.