Simplify time caching in DATE: header formatting (#2176) #2180

socketpair · 2017-08-08T16:41:24Z

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Issue #2176

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the changes folder
- name it <issue_id>.<type> for example (588.bug)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

socketpair · 2017-08-08T16:42:24Z

Will fix tests and formatting slightly later. Please review my idea.

socketpair · 2017-08-08T16:44:39Z

aiohttp/web_response.py

+    if now != _cached_current_datetime:
+        # Weekday and month names for HTTP date/time formatting;
+        # always English!
+        # Tuples are contants stored in codeobject!


fafhrd91

Lets kill TimeService.

fafhrd91 · 2017-08-08T16:57:45Z

aiohttp/web_response.py

-
+_cached_current_datetime = None
+_cached_formatted_datetime = None
+def get_formatted_datetime():


I think this should be helper method.

Do you mean @staticmethod?

Okay, classmethod

just function in helpers.py

asvetlov · 2017-08-08T17:32:33Z

I have no strong opinion for the PR.
On one hand it simplifies the code, on other it adds global variable.
We have no globals yet and I very like it.
Adding tests might require mocking these global vars (which is not terrible bad but I like to avoid it).

codecov-io · 2017-08-12T13:30:31Z

Codecov Report

Merging #2180 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #2180      +/-   ##
=========================================
+ Coverage   97.19%   97.2%   +<.01%     
=========================================
  Files          39      39              
  Lines        8227    8184      -43     
  Branches     1435    1433       -2     
=========================================
- Hits         7996    7955      -41     
+ Misses        102     101       -1     
+ Partials      129     128       -1

Impacted Files	Coverage Δ
aiohttp/test_utils.py	`98.59% <ø> (-0.03%)`	⬇️
aiohttp/web.py	`99.65% <ø> (ø)`	⬆️
aiohttp/web_server.py	`100% <ø> (ø)`	⬆️
aiohttp/web_protocol.py	`88.27% <100%> (+0.46%)`	⬆️
aiohttp/web_response.py	`98.71% <100%> (ø)`	⬆️
aiohttp/helpers.py	`97.26% <100%> (-0.17%)`	⬇️
aiohttp/web_request.py	`99.68% <100%> (-0.01%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 803510b...02f91cd. Read the comment docs.

socketpair · 2017-08-12T14:06:15Z

Please wait. I'm writing test for correct caching in the new function.

socketpair · 2017-08-12T14:42:07Z

And I have found and fixed significant bug (!) in my implementation.

socketpair · 2017-08-12T15:52:48Z

Well, I consider this can be merged right now.

socketpair · 2017-08-12T15:54:09Z

tests/test_helpers.py

+    mock_gmtime.side_effect = gmtime
+
+    mock_time.return_value = 1234567890.998
+    assert helpers.rfc822_formatted_time() == 'Fri, 13 Feb 2009 23:31:30 GMT'


Friday 13 :)

asvetlov · 2017-09-29T17:38:40Z

Ping

@greg-barnett

…ng (#2176) Thanks for @greg-barnett for the code I picked-up.

socketpair · 2017-10-04T12:36:13Z

@asvetlov @fafhrd91

It seems this can be merged.

kxepal · 2017-10-04T12:38:44Z

aiohttp/web_request.py

@@ -402,11 +400,6 @@ def keep_alive(self):
        """Is keepalive enabled by client?"""
        return not self._message.should_close

-    @property
-    def time_service(self):


Wonder if that would be a breaking change.

I don't understand what you mean.

Should be fine. As I remember time_service is not documented

@socketpair Removing public attribute/method/property may break someone's code who depends on it.

@fafhrd91 Ah, cool then.

lock · 2019-10-28T17:04:35Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

socketpair commented Aug 8, 2017

View reviewed changes

fafhrd91 reviewed Aug 8, 2017

View reviewed changes

socketpair closed this Aug 8, 2017

asvetlov mentioned this pull request Aug 10, 2017

issue 2176 Time service removal #2182

Closed

socketpair reopened this Aug 10, 2017

socketpair commented Aug 12, 2017

View reviewed changes

Replaced internal TimeService with simple formatted RFC822 time cachi…

3e26192

…ng (#2176) Thanks for @greg-barnett for the code I picked-up.

kxepal reviewed Oct 4, 2017

View reviewed changes

fafhrd91 approved these changes Oct 4, 2017

View reviewed changes

kxepal approved these changes Oct 4, 2017

View reviewed changes

Merge branch 'master' into time

02f91cd

socketpair merged commit 729eaff into aio-libs:master Oct 5, 2017

socketpair deleted the time branch October 5, 2017 03:56

lock bot added the outdated label Oct 28, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify time caching in DATE: header formatting (#2176) #2180

Simplify time caching in DATE: header formatting (#2176) #2180

socketpair commented Aug 8, 2017 •

edited

Loading

socketpair commented Aug 8, 2017

socketpair Aug 8, 2017

fafhrd91 left a comment

fafhrd91 Aug 8, 2017

socketpair Aug 8, 2017

socketpair Aug 8, 2017

fafhrd91 Aug 8, 2017

asvetlov commented Aug 8, 2017

codecov-io commented Aug 12, 2017 •

edited

Loading

socketpair commented Aug 12, 2017 •

edited

Loading

socketpair commented Aug 12, 2017

socketpair commented Aug 12, 2017

socketpair Aug 12, 2017

asvetlov commented Sep 29, 2017

socketpair commented Oct 4, 2017

kxepal Oct 4, 2017

socketpair Oct 4, 2017 •

edited

Loading

fafhrd91 Oct 4, 2017

kxepal Oct 4, 2017

lock bot commented Oct 28, 2019

Simplify time caching in DATE: header formatting (#2176) #2180

Simplify time caching in DATE: header formatting (#2176) #2180

Conversation

socketpair commented Aug 8, 2017 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

socketpair commented Aug 8, 2017

Choose a reason for hiding this comment

fafhrd91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvetlov commented Aug 8, 2017

codecov-io commented Aug 12, 2017 • edited Loading

Codecov Report

socketpair commented Aug 12, 2017 • edited Loading

socketpair commented Aug 12, 2017

socketpair commented Aug 12, 2017

Choose a reason for hiding this comment

asvetlov commented Sep 29, 2017

socketpair commented Oct 4, 2017

Choose a reason for hiding this comment

socketpair Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lock bot commented Oct 28, 2019

socketpair commented Aug 8, 2017 •

edited

Loading

codecov-io commented Aug 12, 2017 •

edited

Loading

socketpair commented Aug 12, 2017 •

edited

Loading

socketpair Oct 4, 2017 •

edited

Loading