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

Source Command #485

Open
wants to merge 4 commits into
base: master
from

Conversation

@RohanJnr
Copy link
Member

commented Oct 2, 2019

closes #331

Source command implemented as said in the issue.
The following is a screenshot of it being implemented:

sourcecmd

… the cog in __main__.py
Copy link
Member

left a comment

Hey @RohanJnr,

Like mentioned in the issue, the standard approach does not work consistently for our bot, as it does not always return the right starting line and/or code length. I have not looked into why it's happening, although it may be because of decorators passing the function around.

As an example, issuing !source help sends me to https://github.com/python-discord/bot/blob/master/bot/cogs/help.py#L108-L140, which is obviously not where we want to end up. Help may be the only one for which this happens, but I haven't extensively tested all commands yet.

One additional problem is way you reconstruct the path to the .py file: We've recently started using a subpackage approach (directory with __init__.py) to group related functionality together. The command currently does not take that into account and expects all of the extension files to live directly in bot/cogs/.

As an example, issuing !source superstarify yields the link https://github.com/python-discord/bot/tree/master/bot/cogs/superstarify.py#L148-L219. This file does not exist and the function is actually located here: https://github.com/python-discord/bot/blob/master/bot/cogs/superstarify/__init__.py#L148-L219. (Notice that the line numbers are correct in this case.)

I think that if we do add this to the bot, the approach should be robust and not suffer from these edge cases. Additionally, I think that you should consider adding support for subcommands (!source docs get now sends you to the source code for docs, not the get subcommand) and restricting the command to channels like #bot-commands.

I do think the code you wrote is clean and easy to read and I think the functionality would be cool to have.

@RohanJnr

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2019

Hey @RohanJnr,

Like mentioned in the issue, the standard approach does not work consistently for our bot, as it does not always return the right starting line and/or code length. I have not looked into why it's happening, although it may be because of decorators passing the function around.

As an example, issuing !source help sends me to https://github.com/python-discord/bot/blob/master/bot/cogs/help.py#L108-L140, which is obviously not where we want to end up. Help may be the only one for which this happens, but I haven't extensively tested all commands yet.

One additional problem is way you reconstruct the path to the .py file: We've recently started using a subpackage approach (directory with __init__.py) to group related functionality together. The command currently does not take that into account and expects all of the extension files to live directly in bot/cogs/.

As an example, issuing !source superstarify yields the link https://github.com/python-discord/bot/tree/master/bot/cogs/superstarify.py#L148-L219. This file does not exist and the function is actually located here: https://github.com/python-discord/bot/blob/master/bot/cogs/superstarify/__init__.py#L148-L219. (Notice that the line numbers are correct in this case.)

I think that if we do add this to the bot, the approach should be robust and not suffer from these edge cases. Additionally, I think that you should consider adding support for subcommands (!source docs get now sends you to the source code for docs, not the get subcommand) and restricting the command to channels like #bot-commands.

I do think the code you wrote is clean and easy to read and I think the functionality would be cool to have.

Thanks for the suggestion! will work on improving it.

@RohanJnr

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2019

btw by subcommands, do u mean commands within a group?

@RohanJnr

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2019

the only pending work is the pathfinding for the help command. I am not sure how to go about it. The subcommands URL generation works fine except for the tags group, some weared stuff happening. Maybe you can look through my code and find some fixes?

@RohanJnr RohanJnr requested a review from SebastiaanZ Oct 3, 2019
@commands.command(name="source")
async def command_source(self, ctx: commands.Context, command_name: str = None) -> None:
async def command_source(self, ctx: commands.Context, command_name: str = None, sub_command: str = None) -> None:

This comment has been minimized.

Copy link
@NeKitDS

NeKitDS Oct 4, 2019

At the current state, only one command and one subcommand are supported. Why couldn't we do this? : async def command_source(self, ctx: commands.Context, *, command_name: str = None) -> None: This way, end-users can enter anything they wish.

"""View the source of a command."""
if command_name is None:
return await ctx.send("> https://github.com/python-discord/bot")
await ctx.send("> https://github.com/python-discord/bot")

This comment has been minimized.

Copy link
@NeKitDS

NeKitDS Oct 4, 2019

Not sure why wouldn't we do return await ctx.send(...) in one line honestly, but ok if there is a reason.

This comment has been minimized.

Copy link
@sco1

sco1 Oct 4, 2019

Member

If we're annotating that our function returns None then it should return None, not maybe None

This comment has been minimized.

Copy link
@NeKitDS

NeKitDS Oct 4, 2019

Okay, I have just checked in discord.py. send returns a discord.Message instance, well then, we should return None.


command = self.bot.get_command(command_name)

This comment has been minimized.

Copy link
@NeKitDS

NeKitDS Oct 4, 2019

Instead of that sub_command usage, we could do self.bot.get_command(command_name) (almost like before it was deleted, but with new function arguments).

Copy link

left a comment

I have added some suggestions. Planning to do several tests now.

"""Make up the url for the source of the command."""
# Get the source code
src_code_object = command.callback.__code__
file_path = src_code_object.co_filename[5:]

This comment has been minimized.

Copy link
@NeKitDS

NeKitDS Oct 4, 2019

Rapptz's implementation of getting the location was like this: file_name = src_code_object.co_filename and file_path = os.path.relpath(file_name).replace('\\', '/'). I think, should work; will do some tests in a bit.

embed = discord.Embed(colour=discord.Colour.red())
embed.title = "Command Source"
embed.description = f"**{command.name.capitalize()}**\n"
embed.description += f"`{prefix}{command.name}`\n\n {url}"

This comment has been minimized.

Copy link
@NeKitDS

NeKitDS Oct 4, 2019

Shouldn't we send command.qualified_name in the embed?

@RohanJnr

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2019

Updated

if command is None:
await ctx.send("No such command found.")
return

url = self.get_command_url(command)
if command.name == "help":
url = "https://github.com/python-discord/bot/blob/master/bot/cogs/help.py#L475-L490"

This comment has been minimized.

Copy link
@NeKitDS

NeKitDS Oct 4, 2019

In my opinion, we would better send a link without the lines here.

This comment has been minimized.

Copy link
@RohanJnr

RohanJnr Oct 4, 2019

Author Member

Which lines

This comment has been minimized.

Copy link
@NeKitDS

NeKitDS Oct 4, 2019

The #L475-L490 should not be hard-coded in my opinion. I think we should just return the link to the help.py file.

This comment has been minimized.

Copy link
@lemonsaurus

lemonsaurus Oct 4, 2019

Member

I think we should just return the correct lines, but without hardcoding them.

@RohanJnr we can't have any hardcoded edge cases in this solution. Please try to find a solution that solves this without hardcoding.

@NeKitDS

This comment has been minimized.

Copy link

commented Oct 4, 2019

All in all, @SebastiaanZ I think we are ready to merge (though, not sure if we should send the lines in explicit !source help implementation.

@sco1 sco1 added this to Needs review in Bot Tracking Oct 4, 2019
MarkKoz pushed a commit that referenced this pull request Oct 4, 2019
Related to #458

This commit adds a converter that automatically parses ISO-formatted
datetime strings and returns a `datetime.datetime` object. It uses
`dateutil.parser.isoparse` to do the heavy lifting, so it supports
the same formats as this method.

In addition, I have added tests that ensure that it accepts certain
formats and added a description of these 'guaranteed' formats to the
`ISODate.convert` docstring.

This commit should make it easy to implement #485
Copy link
Member

left a comment

This PR is out of date with master and has some conflicts that need to be resolved. @lemonsaurus also rightly noted that we should rather have a solution that's robust instead of one that uses hard coded links for edge cases. Hard coded links will need to be maintained by hand and makes me wonder if more edge cases are going to come up in the future.

@jchristgit jchristgit assigned jchristgit and unassigned jchristgit Oct 11, 2019
@RohanJnr

This comment has been minimized.

Copy link
Member Author

commented Oct 17, 2019

what if I just return the file name for the help command? I will not hard code the file link tho, I can get it by the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Bot Tracking
  
Needs review
6 participants
You can’t perform that action at this time.