Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
PR2: Add new preference and
AnnouncementChecker
class #1509base: shrivaths/changelog-announcement-1
Are you sure you want to change the base?
PR2: Add new preference and
AnnouncementChecker
class #1509Changes from 4 commits
de22a57
69ff1ed
3fc0e75
0db2904
7ab6d8b
7639142
babb86d
6eddcf0
3f99b8b
f8fb1f1
d9947c3
4cb0c6a
3323115
fc856bf
39ed0b6
dce4dab
9e1ebf7
d35f029
21ea789
3cc4188
59f07ae
acdf130
fbb1039
63e329a
cbb661d
9c726af
68cb64b
2c92712
303a2d2
b708717
a679e61
02757e6
090e36f
aca6c7a
c218b10
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:
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, andmatch.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically the same logic, but with regex which is more difficult to understand imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variables
title
anddate
are used before they are defined. This could lead to aNameError
if the first line of the markdown file does not start with##
or_
. Initialize these variables before the loop to avoid potential errors.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roomrys, that's a great suggestion. We can initialize
title
anddate
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: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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.In the above code,
default_path
should be the default location ofbulletin.json
in your project structure. This way, you can override it with an environment variable when necessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:In this code,
os.path.dirname(os.path.abspath(__file__))
gets the directory that the current Python script is in. Theos.path.join()
function then constructs a filepath by joining the base directory with the relative path tobulletin.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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
prefs.prefs
variable should only be read in at opening and written to at closing the program. For intermediate states, we useMainWindow.state
. Theprefs.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 theprefs
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For initialization, we would need to pass in the instance of
MainWindow
to access itsstate
attribute. For example, this is how we initialize theCommandContext
:sleap/sleap/gui/app.py
Lines 123 to 125 in ed77b49
. Here,
self
is the instance ofMainWindow
, and this bit of code resides inMainWindow.__init__
. We just won't know the value ofprefs
if you use it for a default forprevious_announcement_date
and initializeAnnouncementChecker
asAnnouncementChecker()
with no keyword arguments passed in; I expect it will error as an unbound variable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically always intended to return since we catch the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we instead convert both the
latest_date['date']
andself.previous_announcement_date
to a format that allows>
,<
comparison? I see this erroring when theself.previous_announcement_date
>latest_date['date']
and still showing the announcement although it was already seen.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:
This 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
get_latest_announcement
method also opens and reads the JSON file, duplicating the logic incheck_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thedata
in an attribute for access between both functions?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:In 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 incheck_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case where are we calling the
check_for_announcement
function? Since we don't call the function in theget_latest_announcement
,self._latest_data
would beNone
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shrivaths16: You're correct. The
check_for_announcement
function needs to be called beforeget_latest_announcement
to ensure that_latest_data
is notNone
. 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:In 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 callcheck_for_announcement
separately.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this simplifies things, no need to have something (
check_for_announcement
) that we don't really need.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
update_announcement
method does not handle the case where theget_latest_announcement
method returnsNone
. Ifannouncement
isNone
, attempting to index into it will raise aTypeError
. Add error handling to prevent this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
prefs.prefs
variable should only be read in at opening and written to at closing the program. For intermediate states, we useMainWindow.state
. Theprefs.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See other comment: