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: add ChatType builtin filter #356

Merged
merged 14 commits into from
Jul 2, 2020
Merged

feat: add ChatType builtin filter #356

merged 14 commits into from
Jul 2, 2020

Conversation

Birdi7
Copy link
Contributor

@Birdi7 Birdi7 commented Jun 14, 2020

Description

Add the ChatType filter. Old filters were marked as deprecated to retain backward compatibility.

The last filter from #151.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Just used an example

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

fix: rename example file name

fix: str is container also lol. example fixed also
@Birdi7 Birdi7 added the new feature Missing feature label Jun 23, 2020
elif isinstance(obj, CallbackQuery):
obj = obj.message.chat
else:
logger.warning("ChatTypeFilter doesn't support %s as input", type(obj))
Copy link
Member

Choose a reason for hiding this comment

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

Is better to use warnings.warn here

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd do assert isinstance(obj, (Message, CallbackQuery)) before if/elif and in else return False

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really see any point in breaking the whole pipeline of filters. As far as I remember, that's the reason why we didn't use raise NotImplementedError. I'd like to keep the warning message in such case. Remember, that in other filters we don't have such way to handle unexpected input

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 0eb5a47

@Birdi7 Birdi7 requested a review from JrooTJunior June 28, 2020 19:18
@Birdi7
Copy link
Contributor Author

Birdi7 commented Jul 2, 2020

@JrooTJunior I fixed the merge conflicts, should we merge the PR?

@JrooTJunior
Copy link
Member

Yep

@JrooTJunior JrooTJunior merged commit 81b36bd into aiogram:dev-2.x Jul 2, 2020
@MasterGroosha
Copy link
Contributor

Why SUPER_GROUP and not SUPERGROUP ?

@MasterGroosha MasterGroosha mentioned this pull request Oct 6, 2020
11 tasks
dp = Dispatcher(bot)


@dp.message_handler(chat_types=[ChatType.PRIVATE, ChatType.CHANNEL])
Copy link

@kamikazebr kamikazebr Jun 19, 2021

Choose a reason for hiding this comment

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

Typo here, correct to use is chat_type= without 's', right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You’ve viewed the outdated version. Yes, the correct one is chat_type

Choose a reason for hiding this comment

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

I see. Btw in other part of code too not work. I'll find and mark you there or open issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature Missing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants