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

Report the type of change observed in FileWatcher #87

Conversation

daniel-zullo-frequenz
Copy link
Contributor

Previously, the FileWatcher only provided information on the path of the changed file, without indicating whether the change was a creation, modification, or deletion. This commit improves the functionality of the FileWatcher by adding the ability to report the type of change observed. This additional information can be useful when tracking changes to files and understanding the context of the change.

Fixes #53

@daniel-zullo-frequenz daniel-zullo-frequenz requested a review from a team as a code owner May 9, 2023 14:18
@daniel-zullo-frequenz daniel-zullo-frequenz self-assigned this May 9, 2023
@github-actions github-actions bot added part:docs Affects the documentation part:core Affects the core types (`Sender`, `Receiver`, exceptions, etc.) part:tests Affects the unit, integration and performance (benchmarks) tests labels May 9, 2023
Copy link
Contributor

Choose a reason for hiding this comment

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

I propose some improvements to the API.

src/frequenz/channels/util/_file_watcher.py Outdated Show resolved Hide resolved
src/frequenz/channels/util/_file_watcher.py Outdated Show resolved Hide resolved
@daniel-zullo-frequenz daniel-zullo-frequenz force-pushed the feature/report-type-change-file-watcher branch 2 times, most recently from 825baca to 6754e71 Compare May 10, 2023 08:56
@daniel-zullo-frequenz
Copy link
Contributor Author

Updated based on a couple of suggestions from Luca.

src/frequenz/channels/util/_file_watcher.py Outdated Show resolved Hide resolved
src/frequenz/channels/util/_file_watcher.py Outdated Show resolved Hide resolved
tests/utils/test_integration.py Outdated Show resolved Hide resolved
To gradually get rid of the former typing.

Signed-off-by: Daniel Zullo <daniel.zullo@frequenz.com>
@daniel-zullo-frequenz daniel-zullo-frequenz force-pushed the feature/report-type-change-file-watcher branch 3 times, most recently from 8e8bf93 to d3445a1 Compare May 10, 2023 14:49
@daniel-zullo-frequenz
Copy link
Contributor Author

Updated based on latest suggestions from Luca

@daniel-zullo-frequenz
Copy link
Contributor Author

@leandro-lucarella-frequenz I added a trivial fix to correct typos and format in the releases notes (not related to this PR). Can you please check it and re-approve it if LGTY?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just one super minor comment left, but approving anyway because it is just style and for a file that will be cleaned up right after this is merged.

RELEASE_NOTES.md Outdated Show resolved Hide resolved
Previously, the FileWatcher only provided information of the
path of the changed file, without indicating whether the change
was a creation, modification, or deletion. This commit improves the
functionality of the FileWatcher by adding the ability to report
the type of change observed. This additional information can be
useful when tracking changes to files and understanding the
context of the change.

Signed-off-by: Daniel Zullo <daniel.zullo@frequenz.com>
Signed-off-by: Daniel Zullo <daniel.zullo@frequenz.com>
@daniel-zullo-frequenz daniel-zullo-frequenz force-pushed the feature/report-type-change-file-watcher branch from 9477555 to 7469704 Compare May 11, 2023 10:14
Copy link
Contributor

Choose a reason for hiding this comment

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

Besides agreeing to @Marenz suggestion, for me it is fine if it's left for a different PR so we can merge this sooner than later, as it is out of the scope of this PR.

@daniel-zullo-frequenz
Copy link
Contributor Author

Besides agreeing to @Marenz suggestion, for me it is fine if it's left for a different PR so we can merge this sooner than later, as it is out of the scope of this PR.

Ok, I'll merge this PR as it currently is and create an issue to address #87 (comment)

@daniel-zullo-frequenz daniel-zullo-frequenz merged commit 7fd8e46 into frequenz-floss:v0.x.x May 12, 2023
11 checks passed
@daniel-zullo-frequenz daniel-zullo-frequenz deleted the feature/report-type-change-file-watcher branch May 12, 2023 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:core Affects the core types (`Sender`, `Receiver`, exceptions, etc.) part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make FileWatcher return the type of change observed
3 participants