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

Include ITEE Seminar information in !events command. #438

Conversation

Projects
None yet
3 participants
@patrickmeiring
Copy link
Contributor

commented May 25, 2019

As discussed in #uqcs-meta. This change adds seminars run by the School of ITEE to the !events command.
Seminar information is sourced by scraping the https://www.itee.uq.edu.au/seminar-list page on the school website.

Comments:

  • Because information about the speaker is not on the main event listings page, I am having to visit the linked event details pages. As the number of page visits required scales based on the number of events, this may have adverse impacts on performance. On my system the command doesn't take more than a second or two, but I suggest we monitor this and if it becomes a problem, we investigate alternative solutions. These could include any of the following:
    1. Pre-emptively loading and caching speaker information,
    2. Dropping the speaker name from the event,
    3. Turning the script into a 'push' system, rather than a 'pull' system, where event information is automatically pushed into the #events channel on a periodic basis/when it changes.
  • In addition to adding tests for my changes, I have taken the liberty of adding some tests for the other features of the !events command
  • As this is the first time I'm contributing to something written in python, you might want to review my code carefully... I did not know python before today.

@nicklambourne nicklambourne requested review from lighthou and nicklambourne May 25, 2019

@nicklambourne
Copy link
Member

left a comment

Looks great so far, particularly given this is your first contribution in Python. Just a few tiny things to address from me. @TRManderson will love you for making sure everything is typed. Really appreciate the comprehensive testing, too.

Show resolved Hide resolved test/test_events.py Outdated
Show resolved Hide resolved uqcsbot/scripts/events.py
Show resolved Hide resolved uqcsbot/scripts/events.py Outdated
Show resolved Hide resolved uqcsbot/utils/itee_seminar_utils.py Outdated
Show resolved Hide resolved uqcsbot/utils/itee_seminar_utils.py Outdated
Show resolved Hide resolved uqcsbot/utils/itee_seminar_utils.py Outdated
Show resolved Hide resolved test/test_events.py Outdated
Show resolved Hide resolved test/test_events.py Outdated
@patrickmeiring

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

Thanks all for the feedback. @TRManderson @nicklambourne

I basically haven't written any python before, so it's good to learn how I can add those type annotations I was missing. Also thanks for the pointer on File I/O -- I will have a look.

I will address these and come back with an updated PR in a few days when I have time.

@nicklambourne

This comment has been minimized.

Copy link
Member

commented May 27, 2019

The context manager I assume Tom means is this syntax

with open('filename.html') as html_data:
    html_data.do_thing()
Address code-review feedback: include ITEE seminars in events command.
Also include proper encoding of input containing angular brackets and ampersands.
@patrickmeiring

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2019

@TRManderson @nicklambourne Code changes made. Please review the changes I've made and merge if you are happy.

@nicklambourne
Copy link
Member

left a comment

Changes look solid to me. Will wait for @TRManderson to have a look, or run it myself in the testing slack, whichever comes first. Should be good to merge.

@nicklambourne nicklambourne requested review from TRManderson and removed request for lighthou Jun 1, 2019

@nicklambourne nicklambourne merged commit 42f95c6 into UQComputingSociety:master Jun 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.