Skip to content

/cpython

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
+19 −17

Conversation

Reviewers

@tirkarthi tirkarthi

Assignees
No one assigned
Labels
CLA signed awaiting core review
Projects
None yet
Milestone
No milestone
5 participants
@srinivasreddy @tirkarthi @doerwalter @the-knights-who-say-ni @bedevere-bot
@srinivasreddy
Copy link
Contributor

commented Oct 26, 2018 •
edited by bedevere-bot

https://bugs.python.org/issue35078

@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Oct 26, 2018

@bedevere-bot bedevere-bot added the awaiting review label Oct 26, 2018

@tirkarthi

tirkarthi reviewed Oct 26, 2018
View changes

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Oct 26, 2018

Tests could be added for the new change at

cpython/Lib/test/test_calendar.py

Line 561 in 4e3a53b

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

tirkarthi approved these changes Oct 27, 2018
View changes

Copy link
Contributor

left a comment •
edited

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 :)

@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Oct 27, 2018

@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.

Sign in to view
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.

Sign in to view
Copy link
Contributor Author

commented Feb 25, 2019

Sure @doerwalter
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.