Skip to content

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
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
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
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