Skip to content

Conversation

@dovreshef
Copy link

No description provided.

Copy link
Contributor

@Fogapod Fogapod left a comment

Choose a reason for hiding this comment

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

Is there a way to add a test for this?

if nativestr(await self.read_response()) != 'OK':
raise ConnectionError('Invalid Database')

if self.client_name:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if self.client_name:
if self.client_name is not None:

Copy link
Author

Choose a reason for hiding this comment

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

Changed.

parser_class=DefaultParser, reader_read_size=65535,
encoding='utf-8', decode_responses=False,
*, loop=None):
*, loop=None, client_name=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO client_name should be defined before loop as it is application configuration unlike loop, but this is very subjective.

Copy link
Author

Choose a reason for hiding this comment

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

Changed.

@dovreshef
Copy link
Author

dovreshef commented Nov 8, 2020

Is there a way to add a test for this?

Added by setting the client_name on the connection in the tests.

Also rebased on master.

@dovreshef
Copy link
Author

@Fogapod Any chance of reviewing this again?

@Fogapod
Copy link
Contributor

Fogapod commented Mar 8, 2021

@dovreshef yeah, looks good now, but I'm not a maintainer. I just left 2 comments

@dovreshef
Copy link
Author

I see. Thanks.

@NoneGG Can this be merged? Reviewed?

@TheKevJames
Copy link
Contributor

Hi @dovreshef -- just wanted to let you know that my org has recently forked aredis as it appears to have become unmaintained and we've merged in this pull request (and all other open PRs against the aredis repo); in case you find it useful, the fork is here and version 2.0.0 is on PyPI with your changes.

Also wanted to thank you for implementing the redis v5 username & password support, as that made some of my testing a bit easier :)

alisaifee added a commit to alisaifee/coredis that referenced this pull request Jan 16, 2022
alisaifee added a commit to alisaifee/coredis that referenced this pull request Jan 16, 2022
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.

4 participants