-
-
Notifications
You must be signed in to change notification settings - Fork 947
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
Add inspection module to replace print_routes #1661
Conversation
…f falcon applications
Codecov Report
@@ Coverage Diff @@
## master #1661 +/- ##
========================================
Coverage ? 100%
========================================
Files ? 49
Lines ? 3895
Branches ? 606
========================================
Hits ? 3895
Misses ? 0
Partials ? 0
Continue to review full report at Codecov.
|
The code and the tests should be done. |
Codecov Report
@@ Coverage Diff @@
## master #1661 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 49 49
Lines 3928 3928
Branches 611 611
=========================================
Hits 3928 3928 Continue to review full report at Codecov.
|
I've added a new documentation file for the inspect module and updated the quickstart and tutorial files to introduce the inspect module. I think this is ready for a review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a quick test with suffixed responders and they were displayed correctly. Overall the patch looks sound. I still need to spend some time going through the details.
Take your time, it ended up being quite large 👍 The suffixed responders are not treated differently than the normal ones, but the name of the function is correctly printed, like "on_get_with_suffix", but maybe there should be an attribute in the route method class that contains the suffix? Also I was wondering if we should separate the internal routers from the source info print. Now both are under the verbose flag. Maybe we could add an "internal" flag? |
TBH I think it looks good as-is.
That would be great, actually. |
@kgriffs I've split the internal and the verbose options. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. Just noticed a typo
docs/api/inspect.rst
Outdated
The output would be: | ||
|
||
.. code:: | ||
# my_module exposes tha application as a variable named "app" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tha -> the
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, thx!
Hope you don't mind my tweaks... just trying to polish up the prose a bit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a small comment
falcon/inspect.py
Outdated
|
||
Returns: | ||
List[StaticRouteInfo]: The list of static routes of the application. | ||
List[StaticRouteInfo]: The list of static routes that have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A list? as above?
falcon/inspect.py
Outdated
|
||
Returns: | ||
List[SinkInfo]: The list of sinks of the application. | ||
List[SinkInfo]: The list of sinks used by the application. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A list? as above?
falcon/inspect.py
Outdated
|
||
Returns: | ||
List[ErrorHandlerInfo]: The list of error handlers of the application. | ||
List[ErrorHandlerInfo]: The list of error handlers used by the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A list? as above?
No, not at all. I'm not very good with the documentation :( |
Nice! |
Summary of Changes
Create an inspection module to replace the print_route script.
This was inspired by the discussion about #1435 but has evolved in a inspection module that can obtain information regarding the application and create a pretty print of it.
The public interface of the inspection module has different functions that return information regarding an application.
There is a main function to inspect the entire application (routes, middleware, static_routes, sinks and error_handlers) and also individual functions for each.
The returned information are instance of classes (or list of them) which can return a pretty print, since this is more extensible than just a string.
There is also the ability to register inspection of different route class implementations by using a decorator on a inspection function that handles a particular type of router.
This is the output of a sample application
A more verbose output is
View
This is still very much still a proof of concept wip, with documentation and test missing. I wanted to open this PR to get feedback on this.
The main items that are missing are:
Related Issues
This fixes #1435
Pull Request Checklist
This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once; it will save you a few review cycles!
If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing to do.
docs/
.docs/
.versionadded
,versionchanged
, ordeprecated
directives.docs/_newsfragments/
. (Runtowncrier --draft
to ensure it renders correctly.)If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!
PR template inspired by the attrs project.