Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Fix ready() method in FileWatcher #318

Merged
merged 4 commits into from
Sep 11, 2024
Merged

Fix ready() method in FileWatcher #318

merged 4 commits into from
Sep 11, 2024

Conversation

daniel-zullo-frequenz
Copy link
Contributor

@daniel-zullo-frequenz daniel-zullo-frequenz commented Sep 9, 2024 •
edited
Loading

The ready() method now returns False when an exception is set.

Fixes #317

@github-actions github-actions bot added the part:utilities Affects the utility receivers (`Timer`, `Event`, `FileWatcher`) label Sep 9, 2024
@daniel-zullo-frequenz daniel-zullo-frequenz mentioned this pull request Sep 9, 2024
Closed
@daniel-zullo-frequenz
Fix FileWatcher ready() method
acefe46 
The ready() method now returns False when an exception
is set.

Signed-off-by: Daniel Zullo <daniel.zullo@frequenz.com>
@github-actions github-actions bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:docs Affects the documentation labels Sep 10, 2024
@daniel-zullo-frequenz daniel-zullo-frequenz marked this pull request as ready for review September 10, 2024 14:20
@daniel-zullo-frequenz daniel-zullo-frequenz requested a review from a team as a code owner September 10, 2024 14:20
@daniel-zullo-frequenz
Add a note to the FileWatcher documentation
e0cacce 
To clarify the responsibility of the owner of the
FileWatcher instance.

Signed-off-by: Daniel Zullo <daniel.zullo@frequenz.com>
@daniel-zullo-frequenz
Update release-notes with bug-fix for FileWatcher
8728c96 
Signed-off-by: Daniel Zullo <daniel.zullo@frequenz.com>
llucax
llucax reviewed Sep 11, 2024
View reviewed changes
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff LGTM, but the PR still needs some improvements:

  • Update the PR title as this only fix a bug now
  • Update the PR body as it will be part of the merge commit, it doesn't make sense to have outdated, struck out, information.
  • It needs release notes

@llucax llucax added this to the v1.1.1 milestone Sep 11, 2024
@daniel-zullo-frequenz daniel-zullo-frequenz changed the title Fix/improve FileWatcher Fix ready() method in FileWatcher Sep 11, 2024
@daniel-zullo-frequenz
Copy link
Contributor Author

Updated PR tittle and description. The release-notes was already added, do you want me to change something there?

@llucax
Copy link
Contributor

llucax commented Sep 11, 2024

Woops, I guess I was looking commit-by-commit and forget to hit next at some point.

llucax
llucax previously approved these changes Sep 11, 2024
View reviewed changes
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an optional improvement over the release notes, trying to explain a bit more the implications of the bug, which is what users care more about (just knowing that a return False was missed in a method they almost never use is not that informative).

@daniel-zullo-frequenz @llucax
Improve release-notes entry
1d9bb89 
This was suggested in the review to extend
the information in the release-notes entry.

Co-authored-by: Leandro Lucarella <luca@llucax.com>
Signed-off-by: daniel-zullo-frequenz <120166726+daniel-zullo-frequenz@users.noreply.github.com>
@daniel-zullo-frequenz
Copy link
Contributor Author

Just an optional improvement over the release notes, trying to explain a bit more the implications of the bug, which is what users care more about (just knowing that a return False was missed in a method they almost never use is not that informative).

Thanks! Added to suggestion to the release-notes

@daniel-zullo-frequenz daniel-zullo-frequenz added this pull request to the merge queue Sep 11, 2024
Merged via the queue into frequenz-floss:v1.x.x with commit 195a93c Sep 11, 2024
14 checks passed
@daniel-zullo-frequenz daniel-zullo-frequenz deleted the fix/start-stop-file-watcher branch September 11, 2024 13:31
@llucax llucax modified the milestones: v1.1.1, v1.1.2 Sep 13, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@llucax llucax llucax approved these changes

Assignees
No one assigned
Labels
part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:utilities Affects the utility receivers (`Timer`, `Event`, `FileWatcher`)
Projects
None yet
Milestone
v1.1.2
Development

Successfully merging this pull request may close these issues.

FileWatcher stops working after exiting async iterator
2 participants
@daniel-zullo-frequenz @llucax