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

more ECP inference #174

Open
andrewj-brown opened this issue Nov 2, 2023 · 3 comments
Open

more ECP inference #174

andrewj-brown opened this issue Nov 2, 2023 · 3 comments
Labels
enhancement update an existing command or cog for some new functionality good first issue Good for newcomers

Comments

@andrewj-brown
Copy link
Member

andrewj-brown commented Nov 2, 2023

The new courseecp command falls over rather rapidly if something changes from it's expected defaults. It's still capable of returning the ECP, but you have to provide it with every parameter.

This is not ideal for courses that are internally consistent, but just don't line up with the default search parameters. For instance, if you are looking for the ECP for a course that only ever runs in sem 1, and UQCSBot defaults to the current sem, it will fail to find an ECP in sem 2.

For courses that run in both sems, this is a very sensible default. However, we should investigate fallbacks that conduct broader searches for ECPs and just notify the user that the ECP they've been given is from a different sem / campus.

This is primarily just for semester differences - it's probably enough to just have a fallback that checks the other 'main' semester and then gives up.

@andrewj-brown andrewj-brown added enhancement update an existing command or cog for some new functionality good first issue Good for newcomers labels Nov 2, 2023
@ew-b
Copy link
Contributor

ew-b commented Nov 2, 2023

The /courseecp is modelled after /whatsdue and /pastexams. Former of which are fairly semester context dependent where /courseecp is not. A way to fix this would be web scraping to first check what semester(s), campus and mode(s) the course is run in to set as the default inputs instead of assuming current semester, St Lucia, and Internal immediately. So mild logic fixes really

(I did not in fact read the whole issue before typing, SWOTVAC has me dying so don't mind the repeating text)

@ew-b
Copy link
Contributor

ew-b commented Nov 14, 2023

Working on this at the moment:
Inuq-course-utils.py, when get_course_profile_url is called it grabs the most first url on the program course page when offering is None (so when mode, campus is None). Going off that, I can make campus and mode default to None, and when both are None, then set offering to None, giving us the most recent ECP.

For cases where mode is given but campus is not (or vice versa), I need to work out a way to nicely do the web request to get the most recent details since I can't see a logic way out to this.

@49Indium
Copy link
Member

Just a slight comment here while I think of it. The courseecp command currently takes in 4 courses as separate arguments. Taking a single string representing a list of arguments would be best (similar to the latest versions of whatsdue).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement update an existing command or cog for some new functionality good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants