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

Implement basic features with structured layer #1

Merged
merged 8 commits into from
Jul 1, 2021
Merged

Implement basic features with structured layer #1

merged 8 commits into from
Jul 1, 2021

Conversation

hexoul
Copy link
Collaborator

@hexoul hexoul commented Jun 1, 2021

Motivation:

  • Initial commit to make Central Dogma client in Python

Modifications:

`Dogma` ---------- `ProjectService` ----------- `BaseClient`
              |                           |
              ---- `RepositoryService` ----
              |                           |
              ---- `ContentService` -------
  • Implemented synchronous features are as follows:
    • ProjectService
      • List projects
      • Create a project
      • Remove a project
      • Unremove a project
      • Purge a project
    • RepositoryService
      • List repositories
      • Create a repository
      • Remove a repository
      • Unremove a repository
      • Purge a repository
      • Normalize a repository revision
    • ContentService
      • List files
      • Get files
      • Get a file

Result:

  • Above features are available.
  • Examples.
  • README.

@hexoul hexoul requested review from minwoox, okue and ikhoon June 1, 2021 07:37
@hexoul hexoul changed the title Initial commit Implement basic features with structured layer Jun 1, 2021
@minwoox minwoox added this to the 0.1.0 milestone Jun 2, 2021
centraldogma/api_client.py Outdated Show resolved Hide resolved
centraldogma/api_client.py Outdated Show resolved Hide resolved
centraldogma/api_client.py Outdated Show resolved Hide resolved
@ikhoon
Copy link
Contributor

ikhoon commented Jun 4, 2021

The basic structure looks good to me.

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks! @hexoul Let's move on to the next stage!

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks! Left some questions and minor comments. 😄

README.md Show resolved Hide resolved
setup.py Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
centraldogma/base_client.py Show resolved Hide resolved
centraldogma/content_service.py Show resolved Hide resolved
centraldogma/content_service.py Show resolved Hide resolved
centraldogma/content_service.py Show resolved Hide resolved
centraldogma/base_client.py Show resolved Hide resolved
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks! just nits. 😄

kwargs.update(self.configs)
return kwargs

def _request(self, method: str, url: str, **kwargs) -> requests.Response:
Copy link
Member

Choose a reason for hiding this comment

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

only _request() depends on requests, not request()

Yeah but, the return type of request() is requests.Response so it still depends on it?

centraldogma/content_service.py Show resolved Hide resolved
centraldogma/content_service.py Show resolved Hide resolved
centraldogma/content_service.py Show resolved Hide resolved
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks a lot @hexoul, I learned a lot. 😄

return self._request(method, self._create_url(path), **kwargs)

def _create_url(self, path) -> str:
return self.base_url + "/" + self.PATH_PREFIX + path
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return self.base_url + "/" + self.PATH_PREFIX + path
return self.base_url + self.PATH_PREFIX + path

Let's do this and do
PATH_PREFIX = "/api/v1"

Copy link
Member

Choose a reason for hiding this comment

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

Also we might want to normalize the base_url later because if base_url endswith / then there are double /s. 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Good point. I did workaround in constructor to prevent from being double /s. Please check it. https://github.com/line/centraldogma-python/pull/1/files#diff-50828eb36eff8f9de6017b747f7ba14ff8d5ecda0f1f528ee180f3e44574ba6bR24

>>> a = "123/"
>>> a[-1]
'/'
>>> a[:-1]
'123'

Copy link
Member

Choose a reason for hiding this comment

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

Awesome. 👍

@minwoox
Copy link
Member

minwoox commented Jun 29, 2021

@okue Do you have a chance to look at this PR?
If you don't have time, then I think we can just merge this and implement the next feature. 😄

@minwoox
Copy link
Member

minwoox commented Jul 1, 2021

🎉 🎉 🎉
Thanks a lot @hexoul!
Let's go on to the next stage. 😄

@hexoul hexoul deleted the master branch July 12, 2021 18:20
@hexoul hexoul mentioned this pull request Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants