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

bpo-35078:Allow customization of CSS class name of a month in calendar module #10137

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@srinivasreddy
Copy link
Contributor

commented Oct 26, 2018

Show resolved Hide resolved Lib/calendar.py Outdated
@tirkarthi

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

Tests could be added for the new change at

local_month = cal.formatmonthname(2010, 10)

self.assertIn('class="month"', local_month)
cal.cssclass_month_head = "text-center month"
local_month = cal.formatmonthname(2010, 10)
self.assertIn('class="text-center month"', local_month)

A NEWS entry might help with the change.

Thanks for the PR :)

@srinivasreddy srinivasreddy force-pushed the srinivasreddy:35078 branch from b980d09 to 7c2800b Oct 27, 2018

@srinivasreddy srinivasreddy changed the title bpo-35078: Allow customization of css class for the HTML table header bpo-35078:Refactor code in calendar.py module Oct 27, 2018

@srinivasreddy srinivasreddy force-pushed the srinivasreddy:35078 branch from 5ddc8a2 to 6090ae4 Oct 27, 2018

@tirkarthi
Copy link
Contributor

left a comment

I would suggest adding the actual change done to the NEWS entry in addition to the current text like

The month's head CSS class in :class:`calendar.LocaleHTMLCalendar`
is now customizable with attribute ``cssclass_month_head``.

It's more of a personal opinion and would leave it to the reviewer to consider the wording. Other than that LGTM. Thanks for your contribution :)

@srinivasreddy srinivasreddy changed the title bpo-35078:Refactor code in calendar.py module bpo-35078:Allow customization of CSS class name of a month in calendar module Oct 27, 2018

@doerwalter

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2019

The code looks good to me. There's no additional test for LocaleTextCalendar, but that's ok, as there should be no behaviour changes from the patch.

However there's no test for the changed method LocaleHTMLCalendar.formatweekday.

Furthermore the NEWS entry is misleading (and as Serhiy noted on bpo: "We don't usually do pure refactoring changes"). I'd use something like: "Refactor formatweekday, formatmonthname methods in LocaleHTMLCalendar and LocaleTextCalendar classes in calendar module to call the base class methods. This enables customizable CSS classes for LocaleHTMLCalendar.
Patch by Srinivas Reddy Thatiparthy."

If you could add a test for LocaleHTMLCalendar.formatweekday and update the NEWS entry, IMHO the patch is good to go.

@srinivasreddy

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.