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

Create ocflib Tools for Announcements #257

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

Conversation

vicshi06
Copy link

@vicshi06 vicshi06 commented Oct 31, 2023

@vicshi06 vicshi06 linked an issue Oct 31, 2023 that may be closed by this pull request
3 tasks
@vicshi06
Copy link
Author

vicshi06 commented Nov 5, 2023

@ben9583 Hi Ben. Can you take a look and make sure the function behaviors are good? I will implement cache and make the code more efficient later.

ocflib/lab/announcements.py Outdated Show resolved Hide resolved
ocflib/lab/announcements.py Outdated Show resolved Hide resolved
@ben9583
Copy link
Member

ben9583 commented Nov 9, 2023

As for why the tests are failing, one of them is a regular housekeeping task (don't worry about it), and the other two are... from my code from 2 years ago. I'll take a look at why they're failing now.

@ben9583
Copy link
Member

ben9583 commented Nov 9, 2023

Ok I'm blaming the issues with mine on daylight savings happening that day. There only seems to be one commit in which it fails and that doesn't include the most recent one

@vicshi06
Copy link
Author

Ok is there anything I need to do to fix the checks?

@ben9583
Copy link
Member

ben9583 commented Nov 13, 2023

Will take a more thorough look at this tomorrow, but it seems the tests on Jenkins are not working properly since the recent changes

@ben9583
Copy link
Member

ben9583 commented Dec 8, 2023

This is looking pretty much all good, just a couple notes

  • The post IDs should be cached too (in addition to the text which you've done), so that way we don't have to call the GitHub API to scan all of the announcements so often
  • There should be get_last_n_announcements that just returns the IDs and metadata (using caching), we don't need to get all the last n announcement full text contents

Let me know if you have questions of what I mean by this cause I may have worded this a bit weird. I can explain more on specifics

@vicshi06
Copy link
Author

I have changed the structure a little. The script now checks for updated cache whenever get_last_n_announcements and get_last_n_announcements_text are called, so the cache checking behavior is more consistent.

I have also updated the README so people would know how to test individual test file.

Let me know if you have any thoughts! The script should be good to go. I haven't added the tests for the cache since I have not yet thought of a good way to test it. It might make more sense to actually call it from the front end to make sure its behavior is good. But it is not super complex so we don't necessarily need super complex tests.

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.

Create ocflib Tools for Announcements
2 participants