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

Updated SFU modules to slash commands #671

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

chanchantang
Copy link

@chanchantang chanchantang commented Nov 10, 2024

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 chanchantang requested a review from a team as a code owner November 10, 2024 03:46
@modernNeo
Copy link
Member

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.

Copy link
Contributor

@TitanVJ TitanVJ left a comment

Choose a reason for hiding this comment

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

Everything looks good to me. Very nice work @chanchantang !!

@TitanVJ
Copy link
Contributor

TitanVJ commented Nov 15, 2024

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.

@chanchantang
Copy link
Author

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.

self.logger.debug('[SFU outline()] parsing data from get request')
try:
# Main course information
info = data['info']
# Course schedule information
schedule = data['courseSchedule']
except Exception:
self.logger.debug('[SFU outline()] info keys didn\'t exist')
await self._embed_message(interaction, 'SFU Course Outlines', 'SFU Outline Error',
desc=err_desc)
return

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
Copy link
Author

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

@TitanVJ
Copy link
Contributor

TitanVJ commented Nov 16, 2024

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.

self.logger.debug('[SFU outline()] parsing data from get request')
try:
# Main course information
info = data['info']
# Course schedule information
schedule = data['courseSchedule']
except Exception:
self.logger.debug('[SFU outline()] info keys didn\'t exist')
await self._embed_message(interaction, 'SFU Course Outlines', 'SFU Outline Error',
desc=err_desc)
return

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
Copy link
Contributor

TitanVJ commented Nov 16, 2024

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.

@chanchantang
Copy link
Author

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.

Copy link
Member

@modernNeo modernNeo left a comment

Choose a reason for hiding this comment

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

small fixes and then I will merge

self.logger.debug('[SFU sfu()] out sent to server')
return
Copy link
Member

Choose a reason for hiding this comment

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

unnecessary return

await ctx.send(embed=e_obj, reference=ctx.message)
return

async def _construct_fields(self, interaction, data):
self.logger.debug('[SFU outline()] parsing data from get request')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.logger.debug('[SFU outline()] parsing data from get request')
self.logger.debug('[SFU _construct_fields()] parsing data from get request')

@@ -391,18 +165,10 @@ async def outline(self, ctx, *course):
schedule = data['courseSchedule']
except Exception:
self.logger.debug('[SFU outline()] info keys didn\'t exist')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.logger.debug('[SFU outline()] info keys didn\'t exist')
self.logger.debug('[SFU _construct_fields()] info keys didn\'t exist')

@modernNeo modernNeo force-pushed the master branch 10 times, most recently from 08e135d to 2bc735e Compare March 1, 2025 22:33
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.

3 participants