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

feat(handler): requirement class #339

Closed
wants to merge 7 commits into from
Closed

Conversation

uwinx
Copy link
Contributor

@uwinx uwinx commented May 27, 2020

Description

First and not very smart solution for handler requirements.

What is requirements in aiogram handler? They're inspired by awesome fastapi's Dependency. However, in aiogram case, I don't want "Requirements" to be "dependencies", when they're really just a sweet addition. Aiogram will take care of "initializing" those requirements by some set of rules.

Type of change

  • New feature (breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

You can take a look at very simple usage and play out.

Test Configuration:

  • Python version: 3.8

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Implementation

Details that can be unclear

We create cache, exit_stack for every event processing operation. Cache and stack are intermediate internal keys used to solve requirements (they're probably exposed in new middlewares). Filters can have requirements too. generators and asynchronous generators are nicely solved with contextmanager which, I guess, makes their usage "clear".

Limitations

Currently requirement's callback cannot have any parameter which is not Requirement

implement POC of "smart defaults", pin newest pydantic 1.5.1 (resolves
issue with BaseFilter signature inspection)
@codecov
Copy link

codecov bot commented May 27, 2020

Codecov Report

Merging #339 into dev-3.x will decrease coverage by 1.08%.
The diff coverage is 62.20%.

Impacted file tree graph

@@             Coverage Diff             @@
##           dev-3.x     #339      +/-   ##
===========================================
- Coverage   100.00%   98.91%   -1.09%     
===========================================
  Files          212      213       +1     
  Lines         4313     4420     +107     
===========================================
+ Hits          4313     4372      +59     
- Misses           0       48      +48     
Flag Coverage Δ
#unittests 98.91% <62.20%> (-1.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiogram/dispatcher/requirement.py 50.63% <50.63%> (ø)
aiogram/dispatcher/handler/base.py 91.66% <66.66%> (-8.34%) ⬇️
aiogram/dispatcher/event/handler.py 90.90% <75.00%> (-9.10%) ⬇️
aiogram/dispatcher/event/telegram.py 100.00% <100.00%> (ø)

@uwinx uwinx requested a review from JrooTJunior May 27, 2020 09:19
@JrooTJunior JrooTJunior added enhancement Make it better! new feature Missing feature labels May 27, 2020
@JrooTJunior JrooTJunior added this to the 3.0 milestone May 27, 2020
implement POC of requirements for class based handlers
@uwinx uwinx added the breaking This breaks backwards-compatibility label May 28, 2020
fix breaking tests, change signature of BaseHandler
aiogram/dispatcher/event/handler.py Outdated Show resolved Hide resolved
aiogram/dispatcher/event/handler.py Show resolved Hide resolved
aiogram/dispatcher/event/handler.py Show resolved Hide resolved
def __post_init__(self) -> None:
super(HandlerObject, self).__post_init__()
if inspect.isclass(self.callback) and issubclass(self.callback, BaseHandler): # type: ignore
self.awaitable = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you plans to detect awaitable callbacks?

Copy link
Contributor Author

@uwinx uwinx May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

callbacks are naturally inspected in CallableMixin. I didn't actually change the behaviour of inspection, did I? 🤔
This is check determined whether BaseHandler subclass was passed to HandlerObject and seemed redundant to me, since we can check callback once in CallableMixin's __post_init__

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to check it

aiogram/dispatcher/requirement.py Outdated Show resolved Hide resolved
aiogram/dispatcher/requirement.py Outdated Show resolved Hide resolved
aiogram/dispatcher/requirement.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@uwinx uwinx marked this pull request as draft May 31, 2020 03:13
refactor requirements feature
@evgfilim1 evgfilim1 added the 3.x Issue or PR for stable 3.x version label Jun 23, 2020
@uwinx uwinx self-assigned this Jul 21, 2020
@uwinx uwinx requested a review from JrooTJunior July 21, 2020 03:07
@evgfilim1 evgfilim1 added the stale "Dead" or inactive issue label Nov 19, 2020
@evgfilim1 evgfilim1 removed the stale "Dead" or inactive issue label Mar 2, 2021
@evgfilim1 evgfilim1 added this to In progress in 3.0 May 31, 2021
@evgfilim1 evgfilim1 added stale "Dead" or inactive issue and removed enhancement Make it better! labels Aug 3, 2021
@evgfilim1 evgfilim1 removed this from In progress in 3.0 Aug 3, 2021
@evgfilim1 evgfilim1 removed this from the 3.0 milestone Aug 3, 2021
@uwinx
Copy link
Contributor Author

uwinx commented Aug 21, 2021

Closing this after a lot of thinking. This feature unnecessarily makes it hard to maintain. If someone wants their function called before handler gets executed, it should not be done on a new level of update handling from aiogram. It can be done in middlewares/filters already.

@uwinx uwinx closed this Aug 21, 2021
@uwinx uwinx deleted the dev-3.x-require-sugar branch August 21, 2021 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Issue or PR for stable 3.x version breaking This breaks backwards-compatibility new feature Missing feature stale "Dead" or inactive issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants