PR2: Add new preference and AnnouncementChecker class
#1509
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## shrivaths/changelog-announcement-1 #1509 +/- ##
=====================================================================
Coverage ? 73.39%
=====================================================================
Files ? 134
Lines ? 24069
Branches ? 0
=====================================================================
Hits ? 17666
Misses ? 6403
Partials ? 0 ☔ View full report in Codecov by Sentry. |
WalkthroughThe update introduces a system to inform users about new announcements in the SLEAP application. A Python script now generates a JSON file from a markdown bulletin, which the app reads to display recent updates. The GUI tracks the last seen announcement, ensuring users stay informed without repetition. The workflow automation ensures the JSON is updated before website deployment. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Files selected for processing (5)
- docs/bulletin.md (1 hunks)
- docs/make_bulletin_json.py (1 hunks)
- sleap/gui/app.py (4 hunks)
- sleap/gui/web.py (2 hunks)
- sleap/prefs.py (2 hunks)
Files skipped from review due to trivial changes (2)
- docs/bulletin.md
- sleap/prefs.py
Additional comments (Suppressed): 4
sleap/gui/app.py (4)
67-70: The import statement has been updated to include the
AnnouncementCheckerclass fromsleap.gui.web. This is in line with the changes described in the PR summary.152-156: Two new state variables, "announcement last seen date" and "announcement", have been added. These are initialized with the corresponding values from preferences. Ensure that these preference keys are always present in the
prefsobject to avoid KeyError.166-167: An instance of the
AnnouncementCheckerclass is created. This aligns with the PR summary which mentions the addition of this class for checking new announcements.218-222: The state variables "announcement last seen date" and "announcement" are saved back into the preferences before saving the preferences. This ensures that the latest announcement data is stored persistently.
docs/make_bulletin_json.py
Outdated
| def generate_json_file(): | ||
| with open(input_md_file, 'r', encoding='utf-8') as md_file: | ||
| markdown_content = md_file.read() | ||
| bulletin_json = [] | ||
| content = '' | ||
|
|
||
| for line in markdown_content.split('\n'): | ||
| if line.startswith('---'): | ||
| bulletin_json.append({'title': title, 'date': date, 'content':content}) | ||
| content = '' | ||
| elif line.startswith('##'): | ||
| title = line[3:].strip() | ||
| elif line.startswith('_'): | ||
| date = line[1:len(line)-1].strip() | ||
| else: | ||
| content += (line + '\n') | ||
| # Append last section | ||
| bulletin_json.append({'title': title, 'date': date, 'content':content}) |
There was a problem hiding this comment.
This code assumes that the markdown file will always have a specific structure (i.e., sections start with ---, titles start with ##, and dates start with _). If the markdown file does not follow this structure, the script may fail or produce incorrect results. Consider adding error handling to ensure that the markdown file follows the expected structure.
docs/make_bulletin_json.py
Outdated
| else: | ||
| content += (line + '\n') | ||
| # Append last section | ||
| bulletin_json.append({'title': title, 'date': date, 'content':content}) |
There was a problem hiding this comment.
The variables title and date are used before they are defined. This could lead to a NameError if the first line of the markdown file does not start with ## or _. Initialize these variables before the loop to avoid potential errors.
- content = ''
+ title = date = content = ''
There was a problem hiding this comment.
Pretty easy preventative measure. Maybe we can set defaults and raise a warning if the parser never overrides the default value?
There was a problem hiding this comment.
@roomrys, that's a great suggestion. We can initialize title and date with default values and then check if they remain unchanged after parsing the markdown file. If they do, we can raise a warning to alert the user. Here's how we could modify the code:
import json
import warnings
# Set the file paths
input_md_file = 'D:\TalmoLab\sleap\docs\bulletin.md'
output_json_file = 'D:\TalmoLab\sleap\docs\\bulletin.json'
def generate_json_file():
with open(input_md_file, 'r', encoding='utf-8') as md_file:
markdown_content = md_file.read()
bulletin_json = []
content = ''
# Initialize title and date with default values
title = "DEFAULT_TITLE"
date = "DEFAULT_DATE"
for line in markdown_content.split('\n'):
if line.startswith('---'):
bulletin_json.append({'title': title, 'date': date, 'content':content})
content = ''
# Reset title and date to their default values after each section
title = "DEFAULT_TITLE"
date = "DEFAULT_DATE"
elif line.startswith('##'):
title = line[3:].strip()
elif line.startswith('_'):
date = line[1:len(line)-1].strip()
else:
content += (line + '\n')
# Append last section
bulletin_json.append({'title': title, 'date': date, 'content':content})
# Check if title or date were never updated from their default values
if title == "DEFAULT_TITLE" or date == "DEFAULT_DATE":
warnings.warn("The parser did not find a title or date in the markdown file.")
with open(output_json_file, 'w', encoding='utf-8') as json_file:
json.dump(bulletin_json, json_file, ensure_ascii=False, indent=4)
if __name__ == '__main__':
generate_json_file()This way, we can ensure that the script doesn't fail due to uninitialized variables and also alert the user if the markdown file is not formatted as expected.
docs/make_bulletin_json.py
Outdated
| elif line.startswith('##'): | ||
| title = line[3:].strip() | ||
| elif line.startswith('_'): | ||
| date = line[1:len(line)-1].strip() |
There was a problem hiding this comment.
The date extraction logic assumes that the date is surrounded by underscores (_). If the date format changes or if there are additional underscores in the date string, this could lead to incorrect results. Consider using a more robust method for extracting the date, such as regular expressions.
There was a problem hiding this comment.
We plan on sticking with this format, but perhaps we should add a test that the current bulletin.md changes can be parsed correctly. I'm curious, can you give an example of how you would parse the date?
There was a problem hiding this comment.
@roomrys: Sure, I can provide an example using regular expressions which is a more robust way to parse the date. Here's how you could modify your code:
import re
# ...
for line in markdown_content.split('\n'):
if line.startswith('---'):
bulletin_json.append({'title': title, 'date': date, 'content':content})
content = ''
elif line.startswith('##'):
title = line[3:].strip()
else:
match = re.search(r'_([^_]+)_', line)
if match:
date = match.group(1).strip()
else:
content += (line + '\n')In this code, re.search(r'_([^_]+)_', line) looks for a pattern where there are any characters between two underscores. The parentheses in the regular expression create a group, and match.group(1) gets the contents of the first group. This will correctly extract the date even if there are additional underscores in the line.
For testing, you could create a separate function that takes a string as input and returns the parsed title, date, and content. Then, you can write tests that call this function with different inputs and check that the output is as expected.
There was a problem hiding this comment.
Basically the same logic, but with regex which is more difficult to understand imo.
There was a problem hiding this comment.
@roomrys: I understand your concern. Regular expressions can indeed be a bit difficult to grasp initially, but they offer a lot of flexibility and robustness when it comes to text parsing. In this case, the regular expression is used to ensure that the date extraction works correctly even if there are additional underscores in the line.
However, if you prefer to stick with the current approach, it's important to make sure that the format of the dates in the markdown file remains consistent. Any changes could potentially break the date extraction logic. As you suggested earlier, adding tests to verify that the parsing works correctly would be a good way to catch any issues early on.
If you have any other questions or need further clarification, feel free to ask!
sleap/gui/web.py
Outdated
|
|
||
|
|
||
| REPO_ID = "talmolab/sleap" | ||
| ANALYTICS_ENDPOINT = "https://analytics.sleap.ai/ping" | ||
| BULLETIN_JSON = "D:\TalmoLab\sleap\docs\\bulletin.json" |
There was a problem hiding this comment.
The path to the bulletin.json file is hardcoded. This could lead to issues if the file is not located at the specified path on a different machine or environment. Consider making this a configurable option, possibly through an environment variable or configuration file.
- BULLETIN_JSON = "D:\TalmoLab\sleap\docs\\bulletin.json"
+ BULLETIN_JSON = os.getenv('BULLETIN_JSON_PATH', default_path)In the above code, default_path should be the default location of bulletin.json in your project structure. This way, you can override it with an environment variable when necessary.
There was a problem hiding this comment.
I know this is for testing purposes as we don't have the bulletin online yet, but to allow others (me) to test your branch as well, can we instead use relative paths to the location of BULLETIN_JSON?
There was a problem hiding this comment.
@roomrys: Absolutely, using relative paths is a good idea for portability and ease of testing. Here's how you can modify the BULLETIN_JSON path to be relative to the current file's directory:
import os
BASE_DIR = os.path.dirname(os.path.abspath(__file__))
BULLETIN_JSON = os.path.join(BASE_DIR, "..", "docs", "bulletin.json")In this code, os.path.dirname(os.path.abspath(__file__)) gets the directory that the current Python script is in. The os.path.join() function then constructs a filepath by joining the base directory with the relative path to bulletin.json.
This way, regardless of where you run the script from, it will always correctly locate bulletin.json as long as the relative structure of the project remains the same.
sleap/gui/web.py
Outdated
| def check_for_announcement(self) -> bool: | ||
| """Returns if new announcements are available.""" | ||
| try: | ||
| # Attempt to open the file in read mode | ||
| with open(self.bulletin_json_path, 'r', encoding='utf-8') as jsf: | ||
| # Load the JSON data into a Python data structure | ||
| data = json.load(jsf) | ||
| latest_data = data[0] | ||
|
|
||
| if latest_data['date'] != self.previous_announcement_date: | ||
| return True | ||
| except FileNotFoundError: | ||
| return False |
There was a problem hiding this comment.
The check_for_announcement method opens and reads the JSON file every time it's called. This could be inefficient if the method is called frequently. Consider reading the file once and storing the data in memory, then updating it as needed.
There was a problem hiding this comment.
@coderabbitai This will eventually be a ping yielding an online JSON response, would that change your response? Otherwise, what would you suggest with this new information?
There was a problem hiding this comment.
@roomrys If the JSON data is going to be fetched from an online source, then it's reasonable to fetch the data each time you need it. However, if the frequency of checking for announcements is high, you might still want to consider caching the response and updating it at regular intervals to reduce network traffic and latency.
Here's a simple way to implement caching:
import time
@attr.s(auto_attribs=True)
class AnnouncementChecker:
"""Checker for new announcements on the bulletin page of sleap."""
bulletin_json_path: str = BULLETIN_JSON
previous_announcement_date: str = prefs.prefs["announcement last seen date"]
cache_expiry_time: int = 3600 # Cache expiry time in seconds (1 hour)
_cache_timestamp: float = 0.0
_cached_data: Optional[Dict] = None
def _fetch_data(self) -> Dict:
"""Fetches data from the JSON file or online source."""
with open(self.bulletin_json_path, 'r', encoding='utf-8') as jsf:
return json.load(jsf)
def _get_data(self) -> Dict:
"""Gets data from cache if valid, otherwise fetches and updates cache."""
if self._cached_data is None or time.time() - self._cache_timestamp > self.cache_expiry_time:
self._cached_data = self._fetch_data()
self._cache_timestamp = time.time()
return self._cached_data
def check_for_announcement(self) -> bool:
"""Returns if new announcements are available."""
try:
data = self._get_data()
latest_data = data[0]
if latest_data['date'] != self.previous_announcement_date:
return True
except FileNotFoundError:
return FalseThis code will fetch the data from the source only if it's not in the cache or if the cache has expired. The expiry time can be adjusted according to your needs.
sleap/gui/web.py
Outdated
| def get_latest_announcement(self) -> Optional[Tuple[str, str]]: | ||
| """Return latest announcements on the releases page not seen by user.""" | ||
| success = self.check_for_announcement() | ||
| if success: | ||
| # Attempt to open the file in read mode | ||
| with open(self.bulletin_json_path, 'r', encoding='utf-8') as jsf: | ||
| # Load the JSON data into a Python data structure | ||
| data = json.load(jsf) | ||
| latest_data = data[0] | ||
|
|
||
| if latest_data['date'] != self.previous_announcement_date: | ||
| return (latest_data['date'], latest_data['content']) | ||
| return None |
There was a problem hiding this comment.
The get_latest_announcement method also opens and reads the JSON file, duplicating the logic in check_for_announcement. To avoid repeating this operation, consider refactoring these methods to share the file reading and parsing logic.
There was a problem hiding this comment.
Not a bad idea... maybe we can create a helper function just for reading the data and only use this reading function once in check_for_announcement where we can store the data in an attribute for access between both functions?
There was a problem hiding this comment.
@roomrys: That's a great suggestion! By creating a helper function to read the JSON data, we can avoid duplicating code and improve efficiency. Here's how you might refactor the AnnouncementChecker class:
@attr.s(auto_attribs=True)
class AnnouncementChecker:
"""Checker for new announcements on the bulletin page of sleap."""
bulletin_json_path: str = BULLETIN_JSON
previous_announcement_date: str = prefs.prefs["announcement last seen date"]
_latest_data: Optional[Dict[str, str]] = None
def _read_bulletin_data(self) -> Dict[str, str]:
"""Reads the bulletin data from the JSON file."""
try:
with open(self.bulletin_json_path, 'r', encoding='utf-8') as jsf:
data = json.load(jsf)
return data[0]
except FileNotFoundError:
return {}
def check_for_announcement(self) -> bool:
"""Returns if new announcements are available."""
self._latest_data = self._read_bulletin_data()
if self._latest_data and self._latest_data['date'] != self.previous_announcement_date:
return True
return False
def get_latest_announcement(self) -> Optional[Tuple[str, str]]:
"""Return latest announcements on the releases page not seen by user."""
if self._latest_data and self._latest_data['date'] != self.previous_announcement_date:
return (self._latest_data['date'], self._latest_data['content'])
return NoneIn this refactored version, _read_bulletin_data is a helper method that reads the JSON file and returns the latest data. This method is called once in check_for_announcement, and the result is stored in the _latest_data attribute. Then, get_latest_announcement can use this stored data without needing to read the file again.
There was a problem hiding this comment.
In this case where are we calling the check_for_announcement function? Since we don't call the function in the get_latest_announcement, self._latest_data would be None always. So, can we just have one function that checks and returns an announcement if there is one? @coderabbitai do you suggest something else.
There was a problem hiding this comment.
Looks like humans are talking. I'll be quiet for now. If you need me again, just tag @coderabbitai in a new comment.
There was a problem hiding this comment.
@shrivaths16: You're correct. The check_for_announcement function needs to be called before get_latest_announcement to ensure that _latest_data is not None. If we want to simplify the interface and avoid this dependency, we could indeed merge these two functions into one. Here's how you might do it:
@attr.s(auto_attribs=True)
class AnnouncementChecker:
"""Checker for new announcements on the bulletin page of sleap."""
bulletin_json_path: str = BULLETIN_JSON
previous_announcement_date: str = prefs.prefs["announcement last seen date"]
def _read_bulletin_data(self) -> Dict[str, str]:
"""Reads the bulletin data from the JSON file."""
try:
with open(self.bulletin_json_path, 'r', encoding='utf-8') as jsf:
data = json.load(jsf)
return data[0]
except FileNotFoundError:
return {}
def get_latest_announcement(self) -> Optional[Tuple[str, str]]:
"""Return latest announcements on the releases page not seen by user."""
latest_data = self._read_bulletin_data()
if latest_data and latest_data['date'] != self.previous_announcement_date:
return (latest_data['date'], latest_data['content'])
return NoneIn this refactored version, get_latest_announcement calls _read_bulletin_data directly and checks if there's a new announcement. This way, there's no need to call check_for_announcement separately.
There was a problem hiding this comment.
@roomrys does this work, or do we need to store the bulletin JSON file as an attribute after reading it?
There was a problem hiding this comment.
I think this simplifies things, no need to have something (check_for_announcement) that we don't really need.
sleap/gui/web.py
Outdated
| def update_announcement(self): | ||
| """Update the last seen date of announcement in preferences.""" | ||
| announcement = self.get_latest_announcement() | ||
| if announcement is not None: | ||
| prefs.prefs["announcement last seen date"] = announcement[0] | ||
| prefs.prefs["announcement"] = announcement[1] |
There was a problem hiding this comment.
The update_announcement method does not handle the case where the get_latest_announcement method returns None. If announcement is None, attempting to index into it will raise a TypeError. Add error handling to prevent this.
def update_announcement(self):
"""Update the last seen date of announcement in preferences."""
announcement = self.get_latest_announcement()
+ if announcement is None:
+ return
prefs.prefs["announcement last seen date"] = announcement[0]
prefs.prefs["announcement"] = announcement[1]
docs/make_bulletin_json.py
Outdated
| elif line.startswith('##'): | ||
| title = line[3:].strip() | ||
| elif line.startswith('_'): | ||
| date = line[1:len(line)-1].strip() |
There was a problem hiding this comment.
We plan on sticking with this format, but perhaps we should add a test that the current bulletin.md changes can be parsed correctly. I'm curious, can you give an example of how you would parse the date?
docs/make_bulletin_json.py
Outdated
| else: | ||
| content += (line + '\n') | ||
| # Append last section | ||
| bulletin_json.append({'title': title, 'date': date, 'content':content}) |
There was a problem hiding this comment.
Pretty easy preventative measure. Maybe we can set defaults and raise a warning if the parser never overrides the default value?
sleap/gui/web.py
Outdated
|
|
||
|
|
||
| REPO_ID = "talmolab/sleap" | ||
| ANALYTICS_ENDPOINT = "https://analytics.sleap.ai/ping" | ||
| BULLETIN_JSON = "D:\TalmoLab\sleap\docs\\bulletin.json" |
There was a problem hiding this comment.
I know this is for testing purposes as we don't have the bulletin online yet, but to allow others (me) to test your branch as well, can we instead use relative paths to the location of BULLETIN_JSON?
sleap/gui/web.py
Outdated
| if latest_data['date'] != self.previous_announcement_date: | ||
| return True |
There was a problem hiding this comment.
Can we instead convert both the latest_date['date'] and self.previous_announcement_date to a format that allows >, < comparison? I see this erroring when the self.previous_announcement_date > latest_date['date'] and still showing the announcement although it was already seen.
sleap/gui/web.py
Outdated
| def check_for_announcement(self) -> bool: | ||
| """Returns if new announcements are available.""" | ||
| try: | ||
| # Attempt to open the file in read mode | ||
| with open(self.bulletin_json_path, 'r', encoding='utf-8') as jsf: | ||
| # Load the JSON data into a Python data structure | ||
| data = json.load(jsf) | ||
| latest_data = data[0] | ||
|
|
||
| if latest_data['date'] != self.previous_announcement_date: | ||
| return True | ||
| except FileNotFoundError: | ||
| return False |
There was a problem hiding this comment.
@coderabbitai This will eventually be a ping yielding an online JSON response, would that change your response? Otherwise, what would you suggest with this new information?
sleap/gui/web.py
Outdated
| previous_announcement_date: str = prefs.prefs["announcement last seen date"] | ||
|
|
||
| def check_for_announcement(self) -> bool: | ||
| """Returns if new announcements are available.""" |
There was a problem hiding this comment.
Technically always intended to return since we catch the error.
| """Returns if new announcements are available.""" | |
| """Returns True if new announcements are available, otherwise returns False.""" |
sleap/gui/web.py
Outdated
| def get_latest_announcement(self) -> Optional[Tuple[str, str]]: | ||
| """Return latest announcements on the releases page not seen by user.""" | ||
| success = self.check_for_announcement() | ||
| if success: | ||
| # Attempt to open the file in read mode | ||
| with open(self.bulletin_json_path, 'r', encoding='utf-8') as jsf: | ||
| # Load the JSON data into a Python data structure | ||
| data = json.load(jsf) | ||
| latest_data = data[0] | ||
|
|
||
| if latest_data['date'] != self.previous_announcement_date: | ||
| return (latest_data['date'], latest_data['content']) | ||
| return None |
There was a problem hiding this comment.
Not a bad idea... maybe we can create a helper function just for reading the data and only use this reading function once in check_for_announcement where we can store the data in an attribute for access between both functions?
sleap/gui/web.py
Outdated
| prefs.prefs["announcement last seen date"] = announcement[0] | ||
| prefs.prefs["announcement"] = announcement[1] |
There was a problem hiding this comment.
The prefs.prefs variable should only be read in at opening and written to at closing the program. For intermediate states, we use MainWindow.state. The prefs.prefs variable stores the user preferences which are written to a file to be remembered each time the GUI is opened/closed.
There was a problem hiding this comment.
Can we directly use MainWindow.state["announcement last seen date"] or do we have to use it in some other way?
There was a problem hiding this comment.
See other comment:
For initialization, we would need to pass in the instance of
MainWindowto access itsstateattribute. For example, this is how we initialize theCommandContext:Lines 123 to 125 in ed77b49
. Here,
selfis the instance ofMainWindow, and this bit of code resides inMainWindow.__init__. We just won't know the value ofprefsif you use it for a default forprevious_announcement_date, I expect it will error as an unbound variable.
sleap/gui/web.py
Outdated
| """Checker for new announcements on the bulletin page of sleap.""" | ||
|
|
||
| bulletin_json_path: str = BULLETIN_JSON | ||
| previous_announcement_date: str = prefs.prefs["announcement last seen date"] |
There was a problem hiding this comment.
The prefs.prefs variable should only be read in at opening and written to at closing the program. For intermediate states, we use MainWindow.state. The prefs.prefs variable stores the user preferences which are written to a file to be remembered each time the GUI is opened/closed. Also, I don't think that this class would know what the prefs variable is if we ran it in some test code - maybe add both a fixture (for the bulletin.md) and a test that runs through all the methods in the announcement checker?
There was a problem hiding this comment.
Can we directly use MainWindow.state["announcement last seen date"] or do we have to use it in some other way?
There was a problem hiding this comment.
For initialization, we would need to pass in the instance of MainWindow to access its state attribute. For example, this is how we initialize the CommandContext:
Lines 123 to 125 in ed77b49
. Here,
self is the instance of MainWindow, and this bit of code resides in MainWindow.__init__. We just won't know the value of prefs if you use it for a default for previous_announcement_date and initialize AnnouncementChecker as AnnouncementChecker() with no keyword arguments passed in; I expect it will error as an unbound variable.
…gelog-announcement-2
…gelog-announcement-2
…om/talmolab/sleap into shrivaths/changelog-announcement-2
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Files selected for processing (3)
- docs/make_bulletin_json.py (1 hunks)
- sleap/gui/app.py (4 hunks)
- sleap/gui/web.py (2 hunks)
Additional comments (Suppressed): 5
sleap/gui/app.py (4)
67-70: The import of
AnnouncementCheckeris new and seems to be part of the feature addition. Ensure that this class is implemented correctly and handles any potential exceptions, especially when reading from the JSON file.152-156: New state variables "announcement last seen date" and "announcement" are being initialized from preferences. Make sure these preferences are always available or have default values to prevent potential KeyError.
166-169: An instance of
AnnouncementCheckeris created with the application itself as an argument. This could potentially lead to circular dependencies if not handled carefully. Also, ensure that theAnnouncementCheckerclass has proper error handling for any issues that might arise during its operation.220-224: The new preferences "announcement last seen date" and "announcement" are being saved. Ensure that these values are always in a savable format (e.g., serializable) to prevent any errors during saving.
sleap/gui/web.py (1)
- 3-14: The new hunk introduces additional imports and defines two new global variables:
BASE_DIRandBULLETIN_JSON. TheBASE_DIRvariable is used to get the directory of the current file, whileBULLETIN_JSONis used to define the path to the bulletin JSON file. These changes seem appropriate for the new feature being introduced.
docs/make_bulletin_json.py
Outdated
| import json | ||
| import os | ||
|
|
||
| # Set the file paths | ||
| input_md_file = os.path.join(os.path.dirname(__file__), 'bulletin.md') | ||
| output_json_file = os.path.join(os.path.dirname(__file__), 'bulletin.json') |
There was a problem hiding this comment.
The script uses absolute paths to the markdown and JSON files. This could lead to issues if the script is run from a different directory or on a different machine where the file paths may not be the same. Consider using relative paths instead.
- input_md_file = os.path.join(os.path.dirname(__file__), 'bulletin.md')
- output_json_file = os.path.join(os.path.dirname(__file__), 'bulletin.json')
+ input_md_file = 'bulletin.md'
+ output_json_file = 'bulletin.json'
docs/make_bulletin_json.py
Outdated
| if line.startswith('---'): | ||
| bulletin_json.append({'title': title, 'date': date, 'content':content}) | ||
| content = '' | ||
| # Reset title and date to their default values after each section | ||
| title = "DEFAULT_TITLE" | ||
| date = "DEFAULT_DATE" |
There was a problem hiding this comment.
The script assumes that each section in the markdown file starts with ---. If this format changes, the script might not correctly identify the start of a new section. Consider adding a comment to document this assumption or use a more robust method to identify section boundaries.
docs/make_bulletin_json.py
Outdated
| title = line[3:].strip() | ||
| elif line.startswith('_'): | ||
| date = line[1:len(line)-1].strip() | ||
| else: | ||
| content += (line + '\n') |
There was a problem hiding this comment.
The script assumes that the title of a section starts with ## and the date starts with _. If these formats change, the script might not correctly extract the title and date. Consider adding comments to document these assumptions or use a more robust method to extract the title and date.
docs/make_bulletin_json.py
Outdated
| with open(output_json_file, 'w', encoding='utf-8') as json_file: | ||
| json.dump(bulletin_json, json_file, ensure_ascii=False, indent=4) |
There was a problem hiding this comment.
The script writes the generated JSON data to a file without checking if the file already exists. If the file does exist, its contents will be overwritten. Consider checking if the file exists before writing to it and prompt the user for confirmation if it does.
+ if os.path.exists(output_json_file):
+ print(f"File {output_json_file} already exists. Overwrite? (y/n)")
+ response = input().strip().lower()
+ if response != 'y':
+ return
with open(output_json_file, 'w', encoding='utf-8') as json_file:
json.dump(bulletin_json, json_file, ensure_ascii=False, indent=4)
sleap/gui/web.py
Outdated
| ) | ||
|
|
||
|
|
||
| @attr.s(auto_attribs=True) | ||
| class AnnouncementChecker: | ||
| """Checker for new announcements on the bulletin page of sleap.""" | ||
|
|
||
| app: "MainWindow" | ||
| bulletin_json_path: str = BULLETIN_JSON | ||
| previous_announcement_date: str = app.state["announcement last seen date"] | ||
| _latest_data: Optional[Dict[str, str]] = None | ||
|
|
||
| def _read_bulletin_data(self) -> Dict[str, str]: | ||
| """Reads the bulletin data from the JSON file.""" | ||
| try: | ||
| with open(self.bulletin_json_path, 'r', encoding='utf-8') as jsf: | ||
| data = json.load(jsf) | ||
| return data[0] | ||
| except FileNotFoundError: | ||
| return {} | ||
|
|
||
| def get_latest_announcement(self) -> Optional[Tuple[str, str]]: | ||
| """Return latest announcements on the releases page not seen by user.""" | ||
| self._latest_datalatest_data = self._read_bulletin_data() | ||
| if self._latest_data and self._latest_data['date'] != self.previous_announcement_date: | ||
| return (self._latest_data['date'], self._latest_data['content']) | ||
| return None | ||
|
|
||
| def update_announcement(self): | ||
| """Update the last seen date of announcement in preferences.""" | ||
| announcement = self.get_latest_announcement() | ||
| if announcement is None: | ||
| return | ||
| self.app.state["announcement last seen date"] = announcement[0] | ||
| self.app.state["announcement"] = announcement[1] | ||
|
|
||
|
|
||
| def get_analytics_data() -> Dict[str, Any]: |
There was a problem hiding this comment.
This new hunk introduces a new class AnnouncementChecker with methods to read bulletin data from a JSON file, retrieve the latest unseen announcement, and update the last seen date in preferences. However, there's an issue on line 173 where self._latest_datalatest_data should be self._latest_data.
- self._latest_datalatest_data = self._read_bulletin_data()
+ self._latest_data = self._read_bulletin_data()Also, the attribute previous_announcement_date (line 159) is initialized using app.state["announcement last seen date"] which might raise a KeyError if the key doesn't exist in the app state. It would be safer to use app.state.get("announcement last seen date") which will return None if the key doesn't exist.
- previous_announcement_date: str = app.state["announcement last seen date"]
+ previous_announcement_date: str = app.state.get("announcement last seen date")
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (1)
- sleap/gui/web.py (2 hunks)
Additional comments (Suppressed): 1
sleap/gui/web.py (1)
- 3-14: The new hunk introduces additional imports and constants. The
jsonandosmodules are imported, and two new constantsBASE_DIRandBULLETIN_JSONare defined. These changes seem to be necessary for the new feature of tracking and displaying announcements.
sleap/gui/web.py
Outdated
| ) | ||
|
|
||
|
|
||
| @attr.s(auto_attribs=True) | ||
| class AnnouncementChecker: | ||
| """Checker for new announcements on the bulletin page of sleap.""" | ||
|
|
||
| app: "MainWindow" | ||
| bulletin_json_path: str = BULLETIN_JSON | ||
| previous_announcement_date: str = None | ||
| _latest_data: Optional[Dict[str, str]] = None | ||
|
|
||
| def _read_bulletin_data(self) -> Dict[str, str]: | ||
| """Reads the bulletin data from the JSON file.""" | ||
| try: | ||
| with open(self.bulletin_json_path, 'r', encoding='utf-8') as jsf: | ||
| data = json.load(jsf) | ||
| return data[0] | ||
| except FileNotFoundError: | ||
| return {} | ||
|
|
||
| def get_latest_announcement(self) -> Optional[Tuple[str, str]]: | ||
| """Return latest announcements on the releases page not seen by user.""" | ||
| self._latest_data = self._read_bulletin_data() | ||
| if self._latest_data and self._latest_data['date'] != self.previous_announcement_date: | ||
| return (self._latest_data['date'], self._latest_data['content']) | ||
| return None | ||
|
|
||
| def update_announcement(self): | ||
| """Update the last seen date of announcement in preferences.""" | ||
| announcement = self.get_latest_announcement() | ||
| if announcement is None: | ||
| return | ||
| self.app.state["announcement last seen date"] = announcement[0] | ||
| self.app.state["announcement"] = announcement[1] | ||
|
|
There was a problem hiding this comment.
A new class AnnouncementChecker is introduced. This class has methods to read bulletin data from a JSON file, get the latest announcement not seen by the user, and update the last seen date of the announcement in preferences.
However, there's a potential issue with the get_latest_announcement method. It checks if the latest data's date is not equal to the previous announcement date. But it doesn't handle the case where the date key might not exist in _latest_data. This could lead to a KeyError.
Here's a suggested fix:
self._latest_data = self._read_bulletin_data()
- if self._latest_data and self._latest_data['date'] != self.previous_announcement_date:
+ if self._latest_data and 'date' in self._latest_data and self._latest_data['date'] != self.previous_announcement_date:
return (self._latest_data['date'], self._latest_data['content'])
return None…gelog-announcement-2
|
CodeRabbit review skipped By default, CodeRabbit only reviews PRs on the default branch. If you wish to have PRs reviewed on additional branches, you can configure that under the repository settings in the UI. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- tests/data/announcement_checker_bulletin/test_bulletin.json
Files selected for processing (2)
- sleap/gui/web.py (2 hunks)
- tests/gui/test_web.py (2 hunks)
Additional comments: 2
sleap/gui/web.py (1)
- 154-207: The PR objective mentions adding new preferences to the
Preferencesclass, but the provided code does not show any changes toPreferences. Please verify that the changes toPreferencesare included in the PR or update the PR objective accordingly.tests/gui/test_web.py (1)
- 1-13: The imports for
AnnouncementChecker,json,os,CommandContext, andLabelsare correctly added in accordance with the PR objective and the summary.
tests/gui/test_web.py
Outdated
| def test_announcementchecker(bulletin_json_path): | ||
|
|
||
| labels = Labels() | ||
| context = CommandContext.from_labels(labels=labels) | ||
| context.state = {} | ||
| context.state["announcement last seen date"] = "10/10/2023" | ||
| checker = AnnouncementChecker( | ||
| state=context.state, bulletin_json_path=bulletin_json_path | ||
| ) | ||
|
|
||
| # Check if the announcement checker gets the correct date from the app | ||
| assert checker.previous_announcement_date == "10/10/2023" | ||
|
|
||
| # Create dummy JSON file to check | ||
| bulletin_data = [ | ||
| {"title": "title1", "date": "10/12/2023", "content": "New announcement"}, | ||
| {"title": "title2", "date": "10/07/2023", "content": "Old Announcment"}, | ||
| ] | ||
| with open(bulletin_json_path, "w") as test_file: | ||
| json.dump(bulletin_data, test_file) | ||
|
|
||
| # Check if latest announcement is fetched | ||
| announcement = checker.get_latest_announcement() | ||
| assert announcement == ("title1", "10/12/2023", "New announcement") | ||
|
|
||
| # Check if announcement is updated | ||
| checker.update_announcement() | ||
| assert context.state["announcement last seen date"] == "10/12/2023" | ||
| assert context.state["announcement"] == "New announcement" | ||
|
|
||
| # Create dummy JSON file | ||
| bulletin_data = [ | ||
| {"title": "title1", "date": "10/09/2023", "content": "New announcement"}, | ||
| {"title": "title2", "date": "10/07/2023", "content": "Old Announcment"}, | ||
| ] | ||
| with open(bulletin_json_path, "w") as test_file: | ||
| json.dump(bulletin_data, test_file) | ||
|
|
||
| # Check to ensure no new announcement is created | ||
| announcement = checker.get_latest_announcement() | ||
| assert announcement == None |
There was a problem hiding this comment.
The test_announcementchecker function should include teardown code to remove the dummy JSON file created during the test to prevent any side effects on subsequent tests or when the tests are run in a different environment.
def test_announcementchecker(bulletin_json_path):
# ... existing test code ...
# Teardown code to remove the dummy JSON file
os.remove(bulletin_json_path)
sleap/gui/web.py
Outdated
| def update_announcement(self): | ||
| """Update the last seen date of announcement in preferences.""" | ||
| announcement = self.get_latest_announcement() | ||
| if announcement is None: | ||
| return | ||
| self.state["announcement last seen date"] = announcement[1] | ||
| self.state["announcement"] = announcement[2] |
There was a problem hiding this comment.
The update_announcement method assumes that announcement is a tuple with at least two elements. If get_latest_announcement returns a tuple with fewer elements or None, this will raise an IndexError. Consider adding a check to ensure that announcement has the expected structure before accessing its elements.
sleap/gui/web.py
Outdated
| def new_announcement_available(self): | ||
| self._read_bulletin_data() | ||
| latest_date = datetime.strptime(self._latest_data["date"], "%m/%d/%Y") | ||
| previous_date = datetime.strptime(self.previous_announcement_date, "%m/%d/%Y") | ||
| if ( | ||
| self._latest_data | ||
| and self.previous_announcement_date | ||
| and latest_date > previous_date | ||
| ): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
The new_announcement_available property does not handle the case where _latest_data is None, which can occur if the bulletin JSON file is not found or is empty. This will lead to a TypeError when attempting to parse None as a date. Consider adding a check to ensure that _latest_data is not None before attempting to parse dates.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- sleap/prefs.py (2 hunks)
Additional comments: 1
sleap/prefs.py (1)
- 29-36: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [5-36]
The addition of new preferences to the
Preferencesclass aligns with the PR objectives and the summary provided. Ensure that the default values for these preferences are appropriate for their intended use.
| @@ -28,6 +29,8 @@ class Preferences(object): | |||
| "node label size": 12, | |||
| "show non-visible nodes": True, | |||
| "share usage data": True, | |||
| "announcement last seen date": False, | |||
There was a problem hiding this comment.
The default value for "announcement last seen date" is set to False, which is not a date type. Consider using None or a specific date format if the intention is to store a date.
There was a problem hiding this comment.
@talmo I've updated the default value for "announcement last seen date" to None to reflect that it should store a date type. Here's the updated code snippet:
_defaults = {
# ... other defaults ...
"announcement last seen date": None,
# ... other defaults ...
}Please ensure to handle this None value appropriately in the code where the announcement last seen date preference is accessed or updated.
| @@ -28,6 +29,8 @@ | |||
| "node label size": 12, | |||
| "show non-visible nodes": True, | |||
| "share usage data": True, | |||
| "announcement last seen date": False, | |||
| "announcement": False, | |||
There was a problem hiding this comment.
The default value for "announcement" is set to False. If this preference is intended to store a string, consider using an empty string "" or None as a more appropriate default value.
There was a problem hiding this comment.
@talmo I've noted the change for the default value of "announcement" to be an empty string or None. Since I cannot commit code, here's the suggested update for the Preferences class defaults:
_defaults = {
# ... other defaults ...
"announcement last seen date": None,
"announcement": "",
}You can apply this change to the prefs.py file. If you need any further assistance, feel free to ask!
…gelog-announcement-2
…gelog-announcement-2
…gelog-announcement-2
…gelog-announcement-2
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- .github/workflows/website.yml (2 hunks)
Additional comments: 2
.github/workflows/website.yml (2)
12-12: Adding
shrivaths/changelog-announcement-2to the trigger list will allow the workflow to run when changes are pushed to this branch. This seems to be in line with the PR's objectives to support new features related to announcements.52-52: The addition of the
python make_bulletin_json.pystep is necessary to convert the bulletin content into JSON format as per the PR objectives. Ensure that the scriptmake_bulletin_json.pyis present in the repository and that it is designed to be run from thedocsdirectory as the current working directory is changed todocsbefore running the script.Verification successful
The script has successfully located the
make_bulletin_json.pyfile within thedocsdirectory of the repository. This confirms that the file is present where expected and supports the PR's objective to convert bulletin content into JSON format.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the presence of make_bulletin_json.py in the docs directory. fd make_bulletin_json.py --exec test -f {}Length of output: 82
Script:
#!/bin/bash # Verify the presence of make_bulletin_json.py in the repository. fd make_bulletin_json.pyLength of output: 53
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- docs/make_bulletin_json.py (1 hunks)
Additional comments: 2
docs/make_bulletin_json.py (2)
1-6: The script correctly imports necessary modules and sets file paths using
os.path.joinfor better OS compatibility.42-43: The script execution condition is correctly placed to allow the function to be called when the script is run directly.
docs/make_bulletin_json.py
Outdated
| def generate_json_file(): | ||
| with open(input_md_file, "r", encoding="utf-8") as md_file: | ||
| markdown_content = md_file.read() | ||
| bulletin_json = [] | ||
| content = "" | ||
|
|
||
| # Initialize title and date with default values | ||
| title = "DEFAULT_TITLE" | ||
| date = "DEFAULT_DATE" | ||
|
|
||
| for line in markdown_content.split("\n"): | ||
| # Skip if the line begins with # | ||
| if line.startswith("# "): | ||
| continue | ||
| elif line.startswith("---"): | ||
| bulletin_json.append({"title": title, "date": date, "content": content}) | ||
| content = "" | ||
| # Reset title and date to their default values after each section | ||
| title = "DEFAULT_TITLE" | ||
| date = "DEFAULT_DATE" | ||
| elif line.startswith("## "): | ||
| title = line[3:].strip() | ||
| elif line.startswith("_"): | ||
| date = line[1 : len(line) - 1].strip() | ||
| else: | ||
| content += line + "\n" | ||
| # Append last section | ||
| bulletin_json.append({"title": title, "date": date, "content": content}) | ||
|
|
||
| with open("bulletin.json", "w", encoding="utf-8") as json_file: | ||
| json.dump(bulletin_json, json_file, ensure_ascii=False, indent=4) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| generate_json_file() |
There was a problem hiding this comment.
The generate_json_file function is well-structured and follows a clear logic for parsing the Markdown file and generating JSON. However, there are a few points to consider:
- The script assumes a specific Markdown format. Ensure that the Markdown files follow this format strictly, or the script may not parse them correctly.
- The script does not handle the case where the Markdown file does not start with a title or a date. It may be beneficial to add error handling or checks for the expected format.
- The output JSON file path is hardcoded in the
openfunction (line 38), which could be replaced with theoutput_json_filevariable defined earlier for consistency and maintainability. - There is no error handling for file operations which could be problematic if the files don't exist or there are permission issues.
- with open("bulletin.json", "w", encoding="utf-8") as json_file:
+ with open(output_json_file, "w", encoding="utf-8") as json_file:Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def generate_json_file(): | |
| with open(input_md_file, "r", encoding="utf-8") as md_file: | |
| markdown_content = md_file.read() | |
| bulletin_json = [] | |
| content = "" | |
| # Initialize title and date with default values | |
| title = "DEFAULT_TITLE" | |
| date = "DEFAULT_DATE" | |
| for line in markdown_content.split("\n"): | |
| # Skip if the line begins with # | |
| if line.startswith("# "): | |
| continue | |
| elif line.startswith("---"): | |
| bulletin_json.append({"title": title, "date": date, "content": content}) | |
| content = "" | |
| # Reset title and date to their default values after each section | |
| title = "DEFAULT_TITLE" | |
| date = "DEFAULT_DATE" | |
| elif line.startswith("## "): | |
| title = line[3:].strip() | |
| elif line.startswith("_"): | |
| date = line[1 : len(line) - 1].strip() | |
| else: | |
| content += line + "\n" | |
| # Append last section | |
| bulletin_json.append({"title": title, "date": date, "content": content}) | |
| with open("bulletin.json", "w", encoding="utf-8") as json_file: | |
| json.dump(bulletin_json, json_file, ensure_ascii=False, indent=4) | |
| if __name__ == "__main__": | |
| generate_json_file() | |
| def generate_json_file(): | |
| with open(input_md_file, "r", encoding="utf-8") as md_file: | |
| markdown_content = md_file.read() | |
| bulletin_json = [] | |
| content = "" | |
| # Initialize title and date with default values | |
| title = "DEFAULT_TITLE" | |
| date = "DEFAULT_DATE" | |
| for line in markdown_content.split("\n"): | |
| # Skip if the line begins with # | |
| if line.startswith("# "): | |
| continue | |
| elif line.startswith("---"): | |
| bulletin_json.append({"title": title, "date": date, "content": content}) | |
| content = "" | |
| # Reset title and date to their default values after each section | |
| title = "DEFAULT_TITLE" | |
| date = "DEFAULT_DATE" | |
| elif line.startswith("## "): | |
| title = line[3:].strip() | |
| elif line.startswith("_"): | |
| date = line[1 : len(line) - 1].strip() | |
| else: | |
| content += line + "\n" | |
| # Append last section | |
| bulletin_json.append({"title": title, "date": date, "content": content}) | |
| with open(output_json_file, "w", encoding="utf-8") as json_file: | |
| json.dump(bulletin_json, json_file, ensure_ascii=False, indent=4) | |
| if __name__ == "__main__": | |
| generate_json_file() |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sleap/gui/web.py (2 hunks)
- tests/gui/test_web.py (2 hunks)
Additional comments: 2
sleap/gui/web.py (2)
- 212-212: The
get_analytics_datafunction collects various pieces of data for analytics purposes. It's important to ensure that no personally identifiable information (PII) is being collected and transmitted.
- Verify that the data collected complies with privacy policies and user consent.
- Ensure that the data is anonymized and cannot be used to identify individual users.
Verification successful
The
get_analytics_datafunction insleap/gui/web.pycollects the following data:
- SLEAP version
- Python version
- TensorFlow version
- Conda environment name
- Platform information
None of these data points are personally identifiable information (PII). They are related to the software environment and system specifications. Additionally, the function
ping_analyticsincludes a note that it respects user preferences for data collection, which suggests that user consent is considered.Based on the provided script output, it appears that the
get_analytics_datafunction does not collect PII and is designed to comply with privacy policies.* 212-212: The `ping_analytics` function sends analytics data to a remote endpoint. It uses a separate thread to perform the network request.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that no PII is collected by the get_analytics_data function. rg --type python $'get_analytics_data'Length of output: 68
Script:
#!/bin/bash # Correcting the file type and re-running the search for the get_analytics_data function. rg --type py 'def get_analytics_data'Length of output: 100
Script:
#!/bin/bash # Examine the contents of the get_analytics_data function to ensure no PII is collected. rg --type py --context 10 'def get_analytics_data'Length of output: 1154
Script:
#!/bin/bash # Requesting more context lines to view the complete get_analytics_data function. rg --type py --context 30 'def get_analytics_data'Length of output: 3146
- Ensure that the network request respects the user's preference settings regarding data collection.
- Verify that the endpoint is secure and that data is transmitted over HTTPS.
- Consider adding error handling for the thread's execution to catch any exceptions that may occur.
Verification successful
The verification process has confirmed that the
ANALYTICS_ENDPOINTis set to "https://analytics.sleap.ai/ping" in thesleap/gui/web.pyfile, indicating that the endpoint uses HTTPS, which is a secure protocol.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the analytics endpoint is secure and uses HTTPS. rg --type python $'ANALYTICS_ENDPOINT'Length of output: 68
Script:
#!/bin/bash # Correcting the file type specifier and verifying that the analytics endpoint is secure and uses HTTPS. rg --type py 'ANALYTICS_ENDPOINT'Length of output: 158
tests/gui/test_web.py
Outdated
| def test_announcementchecker(): | ||
| labels = Labels() | ||
| context = CommandContext.from_labels(labels=labels) | ||
| context.state = {} | ||
| context.state["announcement last seen date"] = "10/10/2023" | ||
| # Create dummy JSON file to check | ||
| bulletin_data = [ | ||
| {"title": "title1", "date": "10/12/2023", "content": "New announcement"}, | ||
| {"title": "title2", "date": "10/07/2023", "content": "Old Announcment"}, | ||
| ] | ||
| checker = AnnouncementChecker(state=context.state, bulletin_json_data=bulletin_data) | ||
| checker.checked = True | ||
| # Check if the announcement checker gets the correct date from the app | ||
| assert checker.previous_announcement_date == "10/10/2023" | ||
|
|
||
| # Check if latest announcement is fetched | ||
| is_announcement_available = checker.new_announcement_available() | ||
| assert is_announcement_available == True | ||
|
|
||
| # Concatenate the bulletin content to check updated announcement text | ||
| announcement_markdown = "" | ||
| for announcement in bulletin_data: | ||
| announcement_content = "\n".join(announcement["content"].split("\n")) | ||
| announcement_markdown += ( | ||
| "## " + announcement["title"] + "\n" + announcement_content + "\n" | ||
| ) | ||
|
|
||
| # Check if announcement is updated | ||
| checker.update_latest_announcement() | ||
| assert context.state["announcement last seen date"] == "10/12/2023" | ||
| assert context.state["announcement"] == announcement_markdown | ||
|
|
||
| # Create another dummy JSON file | ||
| bulletin_data = [ | ||
| {"title": "title1", "date": "10/09/2023", "content": "New announcement"}, | ||
| {"title": "title2", "date": "10/07/2023", "content": "Old Announcment"}, | ||
| ] | ||
| checker.bulletin_json_data = bulletin_data | ||
| # Check to ensure no new announcement is created | ||
| is_announcement_available = checker.new_announcement_available() | ||
| assert is_announcement_available == False | ||
|
|
There was a problem hiding this comment.
The test_announcementchecker function tests the AnnouncementChecker class. It checks the previous announcement date, the availability of new announcements, and the update of the latest announcement. The test uses hardcoded dates and dummy JSON data for announcements.
- Ensure that the test covers all possible scenarios, including edge cases and failure modes.
- The test should also mock external calls to avoid making actual HTTP requests during testing.
- The use of hardcoded dates should be replaced with relative dates to make the tests less brittle over time.
| is_announcement_available = checker.new_announcement_available() | ||
| assert is_announcement_available == False | ||
|
|
||
|
|
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
sleap/gui/web.py
Outdated
| @attr.s(auto_attribs=True) | ||
| class AnnouncementChecker: | ||
| """Checker for new announcements on the bulletin page of sleap.""" | ||
|
|
||
| state: "GuiState" | ||
| _previous_announcement_date: str = None | ||
| bulletin_json_data: Optional[List[Dict[str, str]]] = None | ||
| json_data_url: str = BULLETIN_JSON_ENDPOINT | ||
| checked: bool = attr.ib(default=False, init=False) | ||
|
|
||
| @property | ||
| def previous_announcement_date(self): | ||
| _previous_announcement_date = self.state["announcement last seen date"] | ||
| return _previous_announcement_date | ||
|
|
||
| def check_for_bulletin_data(self) -> Optional[List[Dict]]: | ||
| """Reads the bulletin data from the JSON file endpoint.""" | ||
| try: | ||
| self.checked = True | ||
| self.bulletin_json_data = requests.get(self.json_data_url).json() | ||
| except (requests.ConnectionError, requests.Timeout): | ||
| self.bulletin_json_data = None | ||
|
|
||
| def new_announcement_available(self) -> bool: | ||
| """Check if latest announcement is available.""" | ||
| if not self.checked: | ||
| self.check_for_bulletin_data() | ||
| if self.bulletin_json_data: | ||
| if self.previous_announcement_date: | ||
| latest_date = datetime.strptime( | ||
| self.bulletin_json_data[0]["date"], "%m/%d/%Y" | ||
| ) | ||
| previous_date = datetime.strptime( | ||
| self.previous_announcement_date, "%m/%d/%Y" | ||
| ) | ||
| if latest_date > previous_date: | ||
| return True | ||
| else: | ||
| return False | ||
| else: | ||
| return True | ||
| else: | ||
| return False | ||
|
|
||
| def update_latest_announcement(self) -> Optional[Tuple[str, str, str]]: | ||
| """Return latest announcements on the releases page not seen by user.""" | ||
| if self.new_announcement_available(): | ||
| announcement_markdown = "" | ||
| for announcement in self.bulletin_json_data: | ||
| announcement_content = "\n".join(announcement["content"].split("\n")) | ||
| announcement_markdown += ( | ||
| "## " + announcement["title"] + "\n" + announcement_content + "\n" | ||
| ) | ||
| self.state["announcement"] = announcement_markdown | ||
| self.state["announcement last seen date"] = self.bulletin_json_data[0][ | ||
| "date" | ||
| ] |
There was a problem hiding this comment.
The search results do not show any sorting logic applied to bulletin_json_data after it's fetched. The code assumes the first item in the JSON data is the latest announcement without explicitly sorting the data. This could lead to incorrect behavior if the data is not already sorted by date in descending order at the source.
Analysis chain
The AnnouncementChecker class has been added to handle the checking and updating of announcements. It includes methods to check for bulletin data, determine if a new announcement is available, and update the latest announcement.
- The previous comment about handling the case where the key "announcement last seen date" might not exist in
self.stateis still valid. This needs to be addressed to prevent aKeyError. - The
check_for_bulletin_datamethod should handle potential exceptions from therequests.getcall more gracefully, including handling non-200 HTTP responses. - The
new_announcement_availablemethod assumes the first announcement in the JSON data is the latest. This should be verified, or the data should be sorted to ensure this is the case. - The
update_latest_announcementmethod updates the state with the latest announcement. Ensure that the state is saved persistently if required.
The script output confirms the line in sleap/gui/web.py where bulletin_json_data is set but does not verify if the JSON data is sorted by date. To ensure the latest announcement is correctly identified, we need to check if the data is sorted or if the code assumes the first item is the latest.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the bulletin JSON data is sorted by date in descending order.
ast-grep --lang python --pattern $'self.bulletin_json_data = requests.get($_).json()'
Length of output: 187
Script:
#!/bin/bash
# Check if there's any sorting logic applied to the bulletin_json_data after it's fetched.
rg "bulletin_json_data" --context 5
Length of output: 4443
docs/make_bulletin_json.py
Outdated
| elif line.startswith("_"): | ||
| date = line[1 : len(line) - 1].strip() |
There was a problem hiding this comment.
This probably needs a stronger check, like a date format regexp or another more explicit indicator. There are lots of cases where we might start a line with "_" in the main content.
There was a problem hiding this comment.
I have added a regex that checks if the entire line has only the date content and it is in a certain format.
| @@ -28,6 +29,8 @@ | |||
| "node label size": 12, | |||
| "show non-visible nodes": True, | |||
| "share usage data": True, | |||
| "announcement last seen date": False, | |||
| "announcement": False, | |||
| @@ -28,6 +29,8 @@ class Preferences(object): | |||
| "node label size": 12, | |||
| "show non-visible nodes": True, | |||
| "share usage data": True, | |||
| "announcement last seen date": False, | |||
talmo
changed the title
AnnoucementChecker classAnnouncementChecker class
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (3)
- docs/make_bulletin_json.py (1 hunks)
- sleap/gui/web.py (2 hunks)
- tests/gui/test_web.py (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- sleap/gui/web.py
- tests/gui/test_web.py
Additional comments: 2
docs/make_bulletin_json.py (2)
- 9-9: Ensure the regex pattern for dates matches the expected format used in
bulletin.md.Verification successful
The regex pattern
^_(\d{2}/\d{2}/\d{4})_$used inmake_bulletin_json.pycorrectly matches the date format found inbulletin.md, as evidenced by the output from thergcommand which found dates in the format09/15/2023and09/10/2023.* 44-45: The script is executed if it's the main module, which is standard practice.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the date format in bulletin.md matches the regex pattern. rg --type md "^_(\d{2}/\d{2}/\d{4})_$" docs/bulletin.mdLength of output: 83
| for line in markdown_content.split("\n"): | ||
| # Skip if the line begins with # | ||
| if line.startswith("# "): | ||
| continue | ||
| elif line.startswith("---"): | ||
| bulletin_json.append({"title": title, "date": date, "content": content}) | ||
| content = "" | ||
| # Reset title and date to their default values after each section | ||
| title = "DEFAULT_TITLE" | ||
| date = "DEFAULT_DATE" | ||
| elif line.startswith("## "): | ||
| title = line[3:].strip() | ||
| elif re.match(date_pattern, line): | ||
| date = line[1 : len(line) - 1].strip() | ||
| else: | ||
| content += line + "\n" |
There was a problem hiding this comment.
The loop processing the markdown content should handle edge cases, such as multiple consecutive newlines or markdown formatting within the content.
Consider adding additional logic to handle edge cases in markdown formatting.
| with open("_static/bulletin.json", "w") as json_file: | ||
| json.dump(bulletin_json, json_file, indent=4) |
There was a problem hiding this comment.
Tip
Codebase Verification
The _static directory does exist within the docs directory of the repository. However, the script output did not provide any instances of build or deployment scripts referencing the _static directory, which means there is no evidence from the script output that such scripts are responsible for creating the _static directory or setting its permissions.
Given this information, the hardcoded path in the make_bulletin_json.py script should be updated to reflect the correct relative path from the script's location to the _static directory. Additionally, the absence of write permissions should be addressed, possibly by modifying the deployment process to ensure the directory is writable when the script is executed.
Analysis chain
The JSON file is written to a hardcoded path. Ensure this path is correct and the file has the proper permissions to be written to.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the _static directory exists and has write permissions.
if [ -w "_static/" ]; then
echo "_static directory is writable."
else
echo "_static directory is not writable or does not exist."
fi
Length of output: 136
Script:
#!/bin/bash
# Search for the _static directory within the repository
fd _static
# Check for build or deployment scripts that might create the _static directory
fd --extension sh --exec grep -H "_static" {}
Length of output: 74
Description
Adding the appropriate preferences for the date of the last seen announcement and updating the
app.pyclass to update the preference of saving the last seen announcement date. Further, creating a classAnnouncementCheckerinweb.pyto check and update the announcement date when a new one is published.This is the second PR of the stack that solves the discussion #1492
Types of changes
Does this address any currently open issues?
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
New Features
Documentation
Automation
User Preferences