search

UQComputingSociety / uqcsbot-slack

:mortar_board: UQCSbot: our friendly little Slack bot
https://slack.uqcs.org.au
MIT License
55 stars 44 forks source link

Added extra holidays #535

Closed LauchieHarvey closed 4 years ago

LauchieHarvey commented 4 years ago

This is my first pull request so bear with me :) I am not sure what branch to merge it to so I just put master :/ This contains a new .csv file which has geeky holidays in it. I also updated the holiday script to read from the .csv so that the holidays will actually show. I didn't know what directory was best to put the .csv in so I just have a constant variable in the holiday.py file referencing the path of the .csv. For now it is in the scripts directory but that can easily be changed. I tried to test it and I think from what I could tell it passed all of the tests, but I couldn't figure out how to actually get the 'holiday' message to display.

nicklambourne commented 4 years ago

Okay, a few things:

  1. Having work on the master branch of your fork of the repo is fine, as is making PRs from there.
  2. As I mentioned in #411, you can create a new directory at uqcsbot/uqcsbot/static to put the CSV in.
  3. Make sure the reference path you use is relative to the uqcsbot root directory.
  4. You may have to change the cron settings for the job (in the decorator) to get it to run while you're looking at it... Don't forget to test it out in our test, if you don't have access to that, DM me your email on regular Slack and I'll add you.
  5. It's currently failing Jenkins checks because of error code F541: https://jenkins.uqcs.org/job/uqcsbot-pr/555/console. These errors (right down the bottom) weren't caused by you, but you could fix them for us as part of the PR, or you can temporarily add --ignore=F541 to the two calls to flake8 in tox.ini and I can fix them later.
nicklambourne commented 4 years ago

Once all of that is fixed, I'll do a proper review.

bradleysigma commented 4 years ago

jenkins retest this please

bradleysigma commented 4 years ago

Apparently flake8 was updated recently, which may be the reason for the F541 error.

LauchieHarvey commented 4 years ago

I have made changes to the tox.ini (added --ignore=F541) file but there is still a failing check. I'll have to do a bit of research on the jenkins system because I've never worked with continuous integration before. @nicklambourne @bradleysigma

katrinafyi commented 4 years ago

It appears that --ignore overrides the default ignore list, leading to warnings in previously-passing code. Using --extend-ignore instead should fix this. https://flake8.pycqa.org/en/latest/user/options.html

nicklambourne commented 4 years ago

Looks like there's just one left: test/test_events.py:87:1: E302 expected 2 blank lines, found 1 Ignore or fix, up to you. Ignore you can just add a comma to the argument to extend-ignore and put that new error code on the end.

LauchieHarvey commented 4 years ago

After your suggestion @nicklambourne and help from @kentonlam it seems to have passed. Sorry I didn't fix the underlying issue. I need to learn more about Jenkins etc. before I have the understanding to solve it :) Thanks for your help and I'll await the PR review.

nicklambourne commented 4 years ago

jenkins retest this please