UrlDispatcher - add_routes returns a list of AbstractRoutes #4141

zlatsic · 2019-10-04T13:49:39Z

What do these changes do?

These changes are supposed to solve the issue #3866, this is just the basic infrastructure of the changes, some discussion about the changes is still required.

Are there changes in behavior for the user?

aiohttp.web.UrlDispatcher.add_routes and aiohttp.web.Application.add_routes return a list of the registered AbstractRoute instances. As a consequence, RouteDef.register now also returns the registered AbstractRoute

Related issue number

#3866

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.bugfix)
- 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."

zlatsic · 2019-10-04T13:51:59Z

First, thank you for the feedback at #3866, but a part of my question was still left unanswered - what about the static routes? I figured it made more sense to make a PR so we can discuss it here where we can reference the code.

codecov-io · 2019-10-04T14:15:16Z

Codecov Report

Merging #4141 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4141      +/-   ##
==========================================
- Coverage   97.57%   97.55%   -0.03%     
==========================================
  Files          43       43              
  Lines        8825     8833       +8     
  Branches     1381     1381              
==========================================
+ Hits         8611     8617       +6     
- Misses         92       93       +1     
- Partials      122      123       +1

Impacted Files	Coverage Δ
aiohttp/web_urldispatcher.py	`99.09% <100%> (ø)`	⬆️
aiohttp/web_app.py	`97.3% <100%> (ø)`	⬆️
aiohttp/web_routedef.py	`100% <100%> (ø)`	⬆️
aiohttp/web_fileresponse.py	`97.72% <0%> (-0.57%)`	⬇️
aiohttp/http_parser.py	`96.88% <0%> (-0.23%)`	⬇️
aiohttp/client_exceptions.py	`100% <0%> (ø)`	⬆️
aiohttp/typedefs.py	`92.59% <0%> (+1.28%)`	⬆️

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 6236536...dcc9b80. Read the comment docs.

zlatsic · 2019-10-04T14:14:27Z

aiohttp/web_routedef.py

@@ -36,7 +39,7 @@

 class AbstractRouteDef(abc.ABC):
    @abc.abstractmethod
-    def register(self, router: UrlDispatcher) -> None:
+    def register(self, router: UrlDispatcher) -> Optional[AbstractRoute]:


This is basically the part I have the most questions about. So far, I've made this return value Optional (and everywhere else where its return value is propagated). The reason for this is because of the StaticDef and its register implementation. Since add_static returns an AbstractResource, do you think it would be sensible for this to be a union of AbstractRoute and AbstractResource for that reason?

StaticResource internally has self._routes which is a dict of ResourceRoute instances.
Perhaps we can extend resource.get_info() to return routes mapping; it can avoid access to private members.

asvetlov

Commented

asvetlov · 2019-10-10T16:40:30Z

aiohttp/web_app.py

-        self.router.add_routes(routes)
+    def add_routes(
+            self, routes: Iterable[AbstractRouteDef]
+    ) -> List[Optional[AbstractRoute]]:


Optional is bad here. The return type should be a strict list.

Suggested change

) -> List[Optional[AbstractRoute]]:

) -> List[AbstractRoute]:

asvetlov · 2019-10-10T16:44:14Z

aiohttp/web_routedef.py

@@ -36,7 +39,7 @@

 class AbstractRouteDef(abc.ABC):
    @abc.abstractmethod
-    def register(self, router: UrlDispatcher) -> None:
+    def register(self, router: UrlDispatcher) -> Optional[AbstractRoute]:


StaticResource internally has self._routes which is a dict of ResourceRoute instances.
Perhaps we can extend resource.get_info() to return routes mapping; it can avoid access to private members.

zlatsic · 2019-10-11T09:12:22Z

All, right, I've updated the changes in the AbstractRouteDef and its subclasses, now register returns a list of registered AbstractRoutes, so now there's no more need for the Optional return values. I've also updated a test for the get_info so that it checks for the field "routes" and whether its values are StaticResources.

I have another question about the tests: I've noticed that there weren't any that check the AbstractRouteDef.register functions, UrlDispatcher.add_routes and Application.add_routes - do you think it's sensible to add them now?

asvetlov

Implementation is good. Please update docs to fix my notes.

asvetlov · 2019-10-14T14:36:11Z

docs/web_reference.rst

@@ -1413,6 +1413,8 @@ duplicated like one using :meth:`Application.copy`.
      The table is a :class:`list` of :class:`RouteDef` items or
      :class:`RouteTableDef`.

+      Returns a :class:`list` of registered :class:`AbstractRoute` instances.


Please use :returns: markup as in the method above.

Please add .. versionchanged:: 3.7 markup, search in docs for the directive usage. The master is not ready for 4.0 release yet, I think we can publish 3.7 earlier.

asvetlov · 2019-10-14T14:36:20Z

docs/web_reference.rst

@@ -1569,6 +1571,8 @@ Router is any object that implements :class:`AbstractRouter` interface.
      The table is a :class:`list` of :class:`RouteDef` items or
      :class:`RouteTableDef`.

+      Returns a :class:`list` of registered :class:`AbstractRoute` instances.


asvetlov · 2019-10-14T14:36:30Z

docs/web_reference.rst

@@ -2080,6 +2084,8 @@ The definition is created by functions like :func:`get` or

      Abstract method, should be overridden by subclasses.

+      Returns a list of registered `AbstractRoute` objects.


And the same

zlatsic · 2019-10-16T11:28:14Z

I've looked up how it's used, hope I got versionchanged right, I put it at the bottom of the method definitions (and the change description), while keeping the returns markup at under the function definition and input argument descriptions.

docs/web_reference.rst

Co-Authored-By: Andrew Svetlov <andrew.svetlov@gmail.com>

asvetlov · 2019-10-18T20:00:37Z

Thanks!

) Co-Authored-By: Andrew Svetlov <andrew.svetlov@gmail.com>. (cherry picked from commit 60e6c22) Co-authored-by: Zlatan <zlatan.sicanica@gmail.com>

) (#4233) Co-Authored-By: Andrew Svetlov <andrew.svetlov@gmail.com>. (cherry picked from commit 60e6c22) Co-authored-by: Zlatan <zlatan.sicanica@gmail.com>

zlatsic · 2019-10-19T16:27:01Z

Don't mention it, glad to help on a library I often use 😄

zlatan added 2 commits October 4, 2019 15:42

UrlDispatcher - add_routes returns a list of AbstractRoutes

4cf126d

add myself to the contributors

1920bd6

zlatsic requested review from asvetlov and webknjaz as code owners October 4, 2019 13:49

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

zlatsic commented Oct 4, 2019

View reviewed changes

asvetlov reviewed Oct 10, 2019

View reviewed changes

zlatan added 2 commits October 11, 2019 10:52

AbstractRouteDef.register returns a list of registered AbstractRoutes

3d1d6b2

test_urldispatch - update get_info test according to new changes

a0694c3

web_routedef: cov fix

ecb1248

asvetlov requested changes Oct 14, 2019

View reviewed changes

docs: update add_routes and registers so they use proper markdown

f046d42

Merge branch 'master' of github.com:aio-libs/aiohttp into urldispatcher

1deafb3

asvetlov reviewed Oct 18, 2019

View reviewed changes

zlatsic and others added 6 commits October 18, 2019 17:37

Update docs/web_reference.rst

1fdb169

Co-Authored-By: Andrew Svetlov <andrew.svetlov@gmail.com>

Update docs/web_reference.rst

43fe35d

Co-Authored-By: Andrew Svetlov <andrew.svetlov@gmail.com>

Update docs/web_reference.rst

2846b45

Co-Authored-By: Andrew Svetlov <andrew.svetlov@gmail.com>

Update docs/web_reference.rst

bfa97ee

Co-Authored-By: Andrew Svetlov <andrew.svetlov@gmail.com>

Update docs/web_reference.rst

16ead65

Co-Authored-By: Andrew Svetlov <andrew.svetlov@gmail.com>

Update docs/web_reference.rst

dcc9b80

Co-Authored-By: Andrew Svetlov <andrew.svetlov@gmail.com>

asvetlov approved these changes Oct 18, 2019

View reviewed changes

asvetlov merged commit 60e6c22 into aio-libs:master Oct 18, 2019

myusko mentioned this pull request May 20, 2020

UrlDispatcher.add_routes to return list of resource objects #3866

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UrlDispatcher - add_routes returns a list of AbstractRoutes #4141

UrlDispatcher - add_routes returns a list of AbstractRoutes #4141

zlatsic commented Oct 4, 2019

zlatsic commented Oct 4, 2019

codecov-io commented Oct 4, 2019 •

edited

Loading

zlatsic Oct 4, 2019

asvetlov Oct 10, 2019

asvetlov left a comment

asvetlov Oct 10, 2019

asvetlov Oct 10, 2019

zlatsic commented Oct 11, 2019

asvetlov left a comment

asvetlov Oct 14, 2019

asvetlov Oct 14, 2019

asvetlov Oct 14, 2019

zlatsic commented Oct 16, 2019

asvetlov commented Oct 18, 2019

zlatsic commented Oct 19, 2019

	) -> List[Optional[AbstractRoute]]:
	) -> List[AbstractRoute]:

		@@ -2080,6 +2084,8 @@ The definition is created by functions like :func:`get` or

		Abstract method, should be overridden by subclasses.

		Returns a list of registered `AbstractRoute` objects.

UrlDispatcher - add_routes returns a list of AbstractRoutes #4141

UrlDispatcher - add_routes returns a list of AbstractRoutes #4141

Conversation

zlatsic commented Oct 4, 2019

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

zlatsic commented Oct 4, 2019

codecov-io commented Oct 4, 2019 • edited Loading

Codecov Report

zlatsic Oct 4, 2019

Choose a reason for hiding this comment

asvetlov Oct 10, 2019

Choose a reason for hiding this comment

asvetlov left a comment

Choose a reason for hiding this comment

asvetlov Oct 10, 2019

Choose a reason for hiding this comment

asvetlov Oct 10, 2019

Choose a reason for hiding this comment

zlatsic commented Oct 11, 2019

asvetlov left a comment

Choose a reason for hiding this comment

asvetlov Oct 14, 2019

Choose a reason for hiding this comment

asvetlov Oct 14, 2019

Choose a reason for hiding this comment

asvetlov Oct 14, 2019

Choose a reason for hiding this comment

zlatsic commented Oct 16, 2019

asvetlov commented Oct 18, 2019

zlatsic commented Oct 19, 2019

codecov-io commented Oct 4, 2019 •

edited

Loading