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

Include dependencies and let some code more readable #67

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

silviolleite
Copy link

Changes

  • Include forgotten tests and locals dependencies
  • Refactor some comparison condition
  • Make the apply_message_translator function more readable

Congratulations! Great lib 🥇

requirements/local.txt Outdated Show resolved Hide resolved
@@ -2,6 +2,7 @@ pytest
pytest-asyncio
pytest-cov
pytest-deadfixtures
pytest-runner
Copy link
Owner

Choose a reason for hiding this comment

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

Can you give me more context about this?

Copy link
Author

Choose a reason for hiding this comment

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

It's required on setup.py at line 91

python setup_requires=['pytest-runner'],

The IDE show an warnning Package requirement 'pytest-runner' is not satisfied.

Copy link
Owner

Choose a reason for hiding this comment

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

I think it's your IDE, I could not find anything in regular environments or circleci. Please revert this change.

@georgeyk
Copy link
Owner

georgeyk commented Aug 5, 2020

hey @silviolleite thanks for your collaboration.
I'll check it later for more feedback. o/

@@ -62,7 +62,8 @@ def on_future__errors(self, future):
def on_loop__stop(self, *args, **kwargs):
logger.info('cancel dispatcher operations ...')

if hasattr(self, '_future'):
future = getattr(self, '_future', None)
if future:
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see any problem with readability here.

@@ -72,7 +68,7 @@ async def deliver(self, raw_message):
async def error_handler(self, exc_info, message):
logger.info('error handler process originated by message={}'.format(message))

if self._error_handler is not None:
if self._error_handler:
Copy link
Owner

Choose a reason for hiding this comment

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

This one as well.

raise ValueError('{} failed to translate message={}'.format(self.message_translator, message))

return processed_message
return translated
Copy link
Owner

Choose a reason for hiding this comment

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

This might override metadata values if we apply the translator several times. I'll check and then we can improve this on the side.

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

Successfully merging this pull request may close these issues.

None yet

2 participants