wall_e icon indicating copy to clipboard operation
wall_e copied to clipboard

Updated SFU modules to slash commands

Open chanchantang opened this issue 1 year ago • 7 comments

Description

In regard to issue #667

Converted def sfu() and def outline() to slash commands. Also rewrote existing commands to be more clean and DRY.

Things to Note:

  • For outline(), I am unsure how to have the optional argument “next”, current setup as an input argument called arg
  • There may be a better way to chunk _construct_fields()
  • _construct_fields() could not use the info and schedule arguments, however it would require interaction to embed an error message. Unsure which is better
  • For _split_course, I return an error using a third return value, there may be a better way to do this
  • Image for thumbnail seems to be broken, so I removed it
  • Room always seems to be TBD
  • Apparently we do not need to use the json library since we can just use request library, I am unsure how while chunking the data
  • For the courses() function, it does not chunk the get request, could be a concern
  • Unsure how to support a help message

PR Checklist

All test cases done in the test cases section are passed except for required arguments now being shown in the Discord UI due to the command being converted from . to /

chanchantang avatar Nov 10 '24 03:11 chanchantang

will review next weekend but @TitanVJ may want to take a crack at it first since he's the one who wrote and knows that command more intimately than I do.

modernNeo avatar Nov 11 '24 11:11 modernNeo

For outline(), I am unsure how to have the optional argument “next”, current setup as an input argument called arg

Maybe turn it into a boolean arg? call it next_term? something like that and have the description updated to say it lets you specifiy if you want to pull next terms information with a yes/no true/false.

_construct_fields() could not use the info and schedule arguments, however it would require interaction to embed an error message. Unsure which is better

What do you mean by this, looks like the code is using the arguments?

Apparently we do not need to use the json library since we can just use request library, I am unsure how while chunking the data

dont know why i made that comment, you can remove it.

Room always seems to be TBD

Looks like the outlines api has been updated and the building code and room number are no longer provided: https://www.sfu.ca/outlines/help/api.html#courseOutline you can just remove those bits of code.

For the courses() function, it does not chunk the get request, could be a concern

The outlines request is chunked because the response is sometimes too large, but that isn't a concern with the calendar api that's used in the courses() command. If there was an issue we would've encountered it by now since this codes been running for years.

@chanchantang my attempt at addressing some of your notes.

TitanVJ avatar Nov 15 '24 21:11 TitanVJ

What do you mean by this, looks like the code is using the arguments?

This part of the code below in outline() can be put inside _construct_fields(), since info and schedule just rely on data. https://github.com/CSSS/wall_e/blob/985095b68335223f7921ac9a3c72d838c7f002ca/wall_e/extensions/sfu.py#L322-L333 However, since that function also possibly embeds a message, we would also need to pass interaction. So the option is either, the current, _construct_fields(data, info, schedule) or _construct_fields(interaction, data).

chanchantang avatar Nov 16 '24 22:11 chanchantang

Also forgot to mention, but I removed the help message, not sure how to support it with the slash commands.

chanchantang avatar Nov 16 '24 22:11 chanchantang

What do you mean by this, looks like the code is using the arguments?

This part of the code below in outline() can be put inside _construct_fields(), since info and schedule just rely on data.

https://github.com/CSSS/wall_e/blob/985095b68335223f7921ac9a3c72d838c7f002ca/wall_e/extensions/sfu.py#L322-L333

However, since that function also possibly embeds a message, we would also need to pass interaction. So the option is either, the current, _construct_fields(data, info, schedule) or _construct_fields(interaction, data).

I'd got w/ the ladder option, seems cleaner overall.

TitanVJ avatar Nov 16 '24 22:11 TitanVJ

Also forgot to mention, but I removed the help message, not sure how to support it with the slash commands.

That's fine, if someones stuck they can use the actual help command.

TitanVJ avatar Nov 16 '24 22:11 TitanVJ

Oh, forgot to mention that the standard error message we embed relies on course_code and course_num, so instead of passing it to _construct_fields() I just changed it.

chanchantang avatar Nov 17 '24 00:11 chanchantang