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

Update client.py #208

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Update client.py #208

wants to merge 5 commits into from

Conversation

xiaokLinux
Copy link

Add a method 'fetch_guild_user' to obtain the user's nickname for a guild.

Add a method 'fetch_guild_user' to obtain the user's nickname for a guild.
Add  method ['message_view', 'list_messages] to get message, and fix 'list_messages' bug that defaults to the second page.
Copy link
Owner

@TWT233 TWT233 left a comment

Choose a reason for hiding this comment

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

besides, it's better to tear this commit into two
(one with fetch_guild_user() and message related methods another, since they are different conceptions enclosed)

khl/client.py Outdated
@@ -159,6 +159,12 @@ async def fetch_guild(self, guild_id: str) -> Guild:
await guild.load()
return guild

async def fetch_guild_user(self, user: Union[User, str], guild_id: str) -> User:
Copy link
Owner

Choose a reason for hiding this comment

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

there is impl for user.view() already: Client.fetch_user()

is this an alias?
would be better just wrap the fetch_user() rather than dup it again

khl/client.py Outdated
return await self.gate.exec_req(api.Message.view(msg_id))

async def list_messages(self,
target_id: str = None,
Copy link
Owner

@TWT233 TWT233 Apr 24, 2023

Choose a reason for hiding this comment

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

  1. it is user-friendlier that expose wrapped concepts to user, rather than underlying mechanism
    on the other side, it is user-friendlier to adapt user's kinds of intention
Suggested change
target_id: str = None,
channel: Union[Channel, str],

channel is a more frequently talked concept in khl.py, user can recall where to get it/how to user it more easily

and user can pass channel obj or raw channel id on condition

khl/client.py Outdated
return await self.gate.exec_req(api.Message.view(msg_id))

async def list_messages(self,
target_id: str = None,
Copy link
Owner

Choose a reason for hiding this comment

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

  1. target id is required param in khl api doc

thus no default value for this

khl/client.py Outdated
Comment on lines 386 to 387
if target_id is not None:
params['target_id'] = target_id
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if target_id is not None:
params['target_id'] = target_id
params['target_id'] = unpack_id(channel)

accordingly

Copy link
Owner

@TWT233 TWT233 left a comment

Choose a reason for hiding this comment

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

some minor problems

khl/client.py Outdated
@@ -148,10 +148,14 @@ def me(self) -> User:
return self._me
raise ValueError('not loaded, please call `await fetch_me()` first')

async def fetch_user(self, user: Union[User, str]) -> User:
async def fetch_user(self, user: Union[User, str], channel: Union[Channel, str] = None) -> User:
Copy link
Owner

Choose a reason for hiding this comment

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

khl/client.py Outdated
@@ -363,3 +367,28 @@ async def fetch_blocked_friends(self) -> List[Friend]:

async def start(self):
await asyncio.gather(self.handle_pkg(), self.gate.run(self._pkg_queue))

async def message_view(self, msg_id: str) -> Dict:
Copy link
Owner

Choose a reason for hiding this comment

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

naming: method name would be better if it is a verb/verb phrase since can convey this is a action/method

Suggested change
async def message_view(self, msg_id: str) -> Dict:
async def view_message(self, msg_id: str) -> Dict:

khl/client.py Outdated

async def message_view(self, msg_id: str) -> Dict:
"""get message detail by message id"""
if len(msg_id) > 0:
Copy link
Owner

Choose a reason for hiding this comment

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

no need to check, for that the kook API will fail if msg id not valid
user is clear that the param is invalid if API fails
but nothing is retern/raised if param not pass the check

Comment on lines +382 to +393
target_id = unpack_id(channel)
params = {'target_id': target_id}
if page_size is not None:
params['page_size'] = page_size
if pin is not None:
params['pin'] = pin
if flag is not None:
params['flag'] = unpack_value(flag)
if msg_id is not None:
params['msg_id'] = msg_id
params['page'] = 0
return await self.gate.exec_req(api.Message.list(**params))
Copy link
Owner

Choose a reason for hiding this comment

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

marked duplicated by pylint here, a small trick:

Suggested change
target_id = unpack_id(channel)
params = {'target_id': target_id}
if page_size is not None:
params['page_size'] = page_size
if pin is not None:
params['pin'] = pin
if flag is not None:
params['flag'] = unpack_value(flag)
if msg_id is not None:
params['msg_id'] = msg_id
params['page'] = 0
return await self.gate.exec_req(api.Message.list(**params))
if isinstance(channel, str):
channel = PublicChannel(id=channel, _gate_=self.gate)
return await channel.list_messages(page_size,pin,flag,msg_id)

@TWT233 TWT233 added the changes requested reviewed but question pops label Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes requested reviewed but question pops
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants