Implement `watch_file` and `watch_repository` #14

ikhoon · 2021-10-27T15:14:54Z

Motivation:

The watch APIs implemented in upstream's client are the most loved features by users.
Python users also want to use the API in this Python client too.

Modifications:

Implement file_watcher and repository_watcher
Apply exception handler
Update docstring

Result:

You can watch the changes in a repository using the watch APIs.

TODO: - [ ] Implement `file_watcher` and `repository_watcher` - [ ] Apply exception handler - [ ] Update docstring

centraldogma/content_service.py

minwoox · 2021-11-07T12:28:51Z

centraldogma/repository_watcher.py

+            else:
+                self._state = WatchState.STARTED
+
+        # FIXME(ikhoon): Replace Thread with Coroutine of asyncio once AsyncClient is implemented.


Are we going to do this before we release 0.1.0?

That seems to depend on how we are going to implement AsyncClient.
If it makes huge breaking changes, it would be better to do it in 0.1.0.

Did you consider asyncio for watcher behind concurrent future? As httpx supports asyncio, using only asyncio without concurrent future may be good in the light of consistency. However, we can even mix them if there are pros. What is the background that concurrent future is chosen over asyncio to implement the watcher in this PR? Multi-threading is well suited for Central Dogma than event loop?

Good question. First, I tried to use Future in asyncio and realized that there is no operation that waits for a result of Future like future.result().
If users need an initial value when starting an application, the blocking operation would be helpful to initialize the application. Even though we change the internal implementation of Watcher, I think, we still use the API of Watcher

there is no operation that waits for a result of Future like future.result().

I couldn't understand this part. There is await reserved keyword. Did I get wrong point? https://docs.python.org/3/library/asyncio-task.html

hexoul

Sorry for late review. 🥲
I left partial comments now and will add if I have time.

centraldogma/base_client.py

centraldogma/repository_watcher.py

centraldogma/data/entry.py

centraldogma/util.py

centraldogma/data/entry.py

centraldogma/query.py

centraldogma/content_service.py

centraldogma/query.py

hexoul · 2021-11-13T03:29:46Z

centraldogma/content_service.py

+            return Entry.text(revision, entry_path, content)
+        elif query_type == QueryType.IDENTITY_JSON or query_type == QueryType.JSON_PATH:
+            if received_entry_type != EntryType.JSON:
+                raise CentralDogmaException(


Would it be better to use server-side exception indicating 500, INTERNAL_SERVER_ERROR instead of CentralDogmaException?

This method is invoked when 200 OK is received. It makes no sense to use 500 INTERNAL_SERVER_ERROR.

But it can be caused by server's abnormal behavior? Hmm then how about UnknownException?

My test code did not fully cover this case.
Added a new test case for this. 893aac5 (#14)

Thanks. WDYT about UnknownException?

centraldogma/content_service.py

centraldogma/data/entry.py

tests/integration/test_watcher.py

hexoul · 2021-11-13T06:14:25Z

centraldogma/dogma.py

@@ -182,3 +192,114 @@ def push(
            the content is supposed to be the new name.
        """
        return self.content_service.push(project_name, repo_name, commit, changes)
+
+    def watch_repository(


Could you add unit tests for new features? Only integration tests were added.

By the way, I don't get a strong advantage of the unit tests. The call path and coverage are rather similar. Is it better to verify Dogma with integration tests and add unit tests for other smaller units? What do you think? 🤔

I understand your stand. Of course, integration tests already cover all. I just would like to improve

consistency as existing functions have both unit test and integration test.

If changed by other developer, they can know in the early stage before integration test.

If it has complex logic in the future, white box testing seems to be needed rather than black box testing.

However, these three points have no strong advantage as you said. Then we are able to keep going without unit test and making the unit test if needed?

Thanks for the good suggestion. 😉
I believe that it's okay without unit tests if the integration tests cover the logic of the changes.

Then we are able to keep going without unit test and making the unit test if needed?

Yeah, I think it's the way to go. 😄

tests/integration/test_watcher.py

centraldogma/watcher.py

hexoul · 2021-11-13T07:16:02Z

centraldogma/repository_watcher.py

+            else:
+                self._state = WatchState.STARTED
+
+        # FIXME(ikhoon): Replace Thread with Coroutine of asyncio once AsyncClient is implemented.


Did you consider asyncio for watcher behind concurrent future? As httpx supports asyncio, using only asyncio without concurrent future may be good in the light of consistency. However, we can even mix them if there are pros. What is the background that concurrent future is chosen over asyncio to implement the watcher in this PR? Multi-threading is well suited for Central Dogma than event loop?

ikhoon · 2021-11-19T03:43:57Z

By the way, why GitHub Actions is not working even though master branch is merged. 🤔

ikhoon · 2021-11-19T03:58:34Z

By the way, why GitHub Actions is not working even though master branch is merged. 🤔

https://github.community/t/github-action-action-in-private-repository/16063
We can't trigger GitHub Actions with a PR on the cross repositories. 😅
Is it time to make this public with a "work in progress" badge? I think it is good to share the developing progress with others.
@syleeeee Any thoughts on this? If you want to open a complete product, I can run a test manually at the moment.

syleeeee · 2021-11-19T04:16:43Z

@ikhoon Sounds good to me. We could add the repository description as WIP after making it public.
Then I'll announce the news on Twitter later when it's all ready :)

ikhoon · 2021-11-19T04:18:45Z

Thanks for the confirmation. I will update the notice into the README and turn this repository into public.
/cc @hexoul @minwoox

ikhoon · 2021-11-22T03:40:12Z

Hmm... reopen button disappeared. 😅

ikhoon · 2021-11-22T03:46:54Z

The relationship between downstream and upstream is disconnected.
Let me refork this repository and reopen a PR.

ikhoon added 3 commits October 28, 2021 00:12

Implement watch_file and watch_repository

2c24746

TODO: - [ ] Implement `file_watcher` and `repository_watcher` - [ ] Apply exception handler - [ ] Update docstring

Implemented repository watcher

6098f1a

Use daemon thread

9d9ea91

This was referenced Oct 29, 2021

Use dedicated Revision type instead of primitive int #15

Open

Use Query type instead of str #16

Open

ikhoon added 4 commits October 29, 2021 18:30

Update docstring

549c0fd

Clean up

c85b6aa

docstring for query

cca9718

new line

0450156

ikhoon added the new feature label Oct 30, 2021

ikhoon added this to the 0.1.0 milestone Oct 30, 2021

ikhoon added 3 commits November 2, 2021 11:53

Use __init__ in the super class

d1cd5c1

Merge branch 'main' into watch-repo

7f16629

log a proper message on an exception

176864a

ikhoon marked this pull request as ready for review November 2, 2021 07:35

hexoul requested review from hexoul and minwoox November 5, 2021 08:20

minwoox reviewed Nov 7, 2021

View reviewed changes

hexoul reviewed Nov 11, 2021

View reviewed changes

centraldogma/base_client.py Outdated Show resolved Hide resolved

centraldogma/repository_watcher.py Outdated Show resolved Hide resolved

centraldogma/repository_watcher.py Outdated Show resolved Hide resolved

centraldogma/data/entry.py Outdated Show resolved Hide resolved

hexoul reviewed Nov 11, 2021

View reviewed changes

centraldogma/util.py Outdated Show resolved Hide resolved

hexoul reviewed Nov 12, 2021

View reviewed changes

centraldogma/data/entry.py Show resolved Hide resolved

centraldogma/query.py Outdated Show resolved Hide resolved

Address comments by @minwoox and @hexoul

e9e6d4a

hexoul reviewed Nov 13, 2021

View reviewed changes

ikhoon added 3 commits November 17, 2021 16:03

Address comments by @hexoul

823d286

Merge branch 'main' into watch-repo

b08a489

Add a test for a invalid query type

893aac5

ikhoon closed this Nov 19, 2021

ikhoon reopened this Nov 19, 2021

Remove branch from pull_request

c0c5caa

Trigger build (Fri Nov 19 12:51:35 KST 2021)

786fe89

ikhoon closed this Nov 19, 2021

ikhoon reopened this Nov 19, 2021

ikhoon closed this Nov 22, 2021

hexoul mentioned this pull request Nov 23, 2021

Implement watch_file and watch_repository #24

Merged

hexoul removed this from the 0.1.1 milestone Jul 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `watch_file` and `watch_repository` #14

Implement `watch_file` and `watch_repository` #14

ikhoon commented Oct 27, 2021 •

edited

minwoox Nov 7, 2021

ikhoon Nov 12, 2021

hexoul Nov 13, 2021

ikhoon Nov 17, 2021

hexoul Nov 19, 2021

hexoul left a comment

hexoul Nov 13, 2021

ikhoon Nov 17, 2021

hexoul Nov 18, 2021 •

edited

ikhoon Nov 19, 2021 •

edited

hexoul Nov 19, 2021

hexoul Nov 13, 2021

ikhoon Nov 17, 2021

hexoul Nov 19, 2021

minwoox Nov 29, 2021

hexoul Nov 13, 2021

ikhoon commented Nov 19, 2021

ikhoon commented Nov 19, 2021

syleeeee commented Nov 19, 2021

ikhoon commented Nov 19, 2021

ikhoon commented Nov 22, 2021

ikhoon commented Nov 22, 2021

Implement watch_file and watch_repository #14

Implement watch_file and watch_repository #14

Conversation

ikhoon commented Oct 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hexoul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hexoul Nov 18, 2021 • edited

Choose a reason for hiding this comment

ikhoon Nov 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon commented Nov 19, 2021

ikhoon commented Nov 19, 2021

syleeeee commented Nov 19, 2021

ikhoon commented Nov 19, 2021

ikhoon commented Nov 22, 2021

ikhoon commented Nov 22, 2021

Implement `watch_file` and `watch_repository` #14

Implement `watch_file` and `watch_repository` #14

ikhoon commented Oct 27, 2021 •

edited

hexoul Nov 18, 2021 •

edited

ikhoon Nov 19, 2021 •

edited