Skip to content

Refactored methods by each module & implemented subscribe module#40

Merged
psy2848048 merged 9 commits into
features/protobuffrom
private/wenli
Oct 19, 2020
Merged

Refactored methods by each module & implemented subscribe module#40
psy2848048 merged 9 commits into
features/protobuffrom
private/wenli

Conversation

@psy2848048

@psy2848048 psy2848048 commented Oct 8, 2020

Copy link
Copy Markdown
Contributor

Implemented

  • Changed onetime payment as protobuf form
  • Implemented as a separated method
  • In case of unwanted input at parameter check, raise custom exception, instead of KeyError
  • Passed tox tests (2.7, 3.x, pypy) (2.7 available!!)
  • Wrote a test accordingly
  • Revised error assertion criteria - deleted
  • Moved exceptions to separate module
  • Implemented subscribe API wrappers

Need to be discussed

- Replace to protobuf vs create additional SDK vs add additional protobuf methods

  • Decided to implement as protobuf for new methods
  • Code

@psy2848048 psy2848048 requested a review from a team October 8, 2020 02:01
@psy2848048 psy2848048 self-assigned this Oct 8, 2020
@nowooj

nowooj commented Oct 8, 2020

Copy link
Copy Markdown

rest client를 호출하고 return 해주는 type도 protobuf에서 response 로 정의한 타입에 매핑하여 넘겨주면 더 사용하기 쉽지 않을까요?

@psy2848048

psy2848048 commented Oct 8, 2020

Copy link
Copy Markdown
Contributor Author

rest client를 호출하고 return 해주는 type도 protobuf에서 response 로 정의한 타입에 매핑하여 넘겨주면 더 사용하기 쉽지 않을까요?

@JoowonYun
나중에 gRPC를 사용할 때, protobuf response를 받아오면 그걸 해당 언어의 dictionary로 바꿔서 보내주는게 SDK의 역할이 아닐까하는;;;
JSON으로 받아오는 지금 REST 환경에서는 JSON으로 받아온 response를 굳이 protobuf로 감싸서 리턴해줄 필요가 있나 싶네요

@nowooj

nowooj commented Oct 8, 2020

Copy link
Copy Markdown

rest client를 호출하고 return 해주는 type도 protobuf에서 response 로 정의한 타입에 매핑하여 넘겨주면 더 사용하기 쉽지 않을까요?

@JoowonYun
나중에 gRPC를 사용할 때, protobuf response를 받아오면 그걸 해당 언어의 dictionary로 바꿔서 보내주는게 SDK의 역할이 아닐까하는;;;
JSON으로 받아오는 지금 REST 환경에서는 JSON으로 받아온 response를 굳이 protobuf로 감싸서 리턴해줄 필요가 있나 싶네요

@psy2848048
관점 차이일거 같긴한데, json response로 넘어온것에 response 변수명을 알 수 없지 않나요? 결국 사용자가 return value가 어떤 값인지 알고 해당 값을 조회해야하는데 response로 명세 된 타입을 return 해주면 setter, getter가 있으니 사용자가 api 명세를 모르고도 원하는 값을 찾아 쓸 수 있는 이점이 있지 않을까요?

@psy2848048

Copy link
Copy Markdown
Contributor Author

rest client를 호출하고 return 해주는 type도 protobuf에서 response 로 정의한 타입에 매핑하여 넘겨주면 더 사용하기 쉽지 않을까요?

@JoowonYun
나중에 gRPC를 사용할 때, protobuf response를 받아오면 그걸 해당 언어의 dictionary로 바꿔서 보내주는게 SDK의 역할이 아닐까하는;;;
JSON으로 받아오는 지금 REST 환경에서는 JSON으로 받아온 response를 굳이 protobuf로 감싸서 리턴해줄 필요가 있나 싶네요

@psy2848048
관점 차이일거 같긴한데, json response로 넘어온것에 response 변수명을 알 수 없지 않나요? 결국 사용자가 return value가 어떤 값인지 알고 해당 값을 조회해야하는데 response로 명세 된 타입을 return 해주면 setter, getter가 있으니 사용자가 api 명세를 모르고도 원하는 값을 찾아 쓸 수 있는 이점이 있지 않을까요?

@JoowonYun
Python도 Getter / Setter 모델을 지향하는군요 👍 감싸서 리턴하겠음다
이렇게 되면 선택지가 repo 분리 vs protobuf 별도 method 로 좁혀지겠군요

@psy2848048

Copy link
Copy Markdown
Contributor Author

@JoowonYun 65eca78

@psy2848048 psy2848048 changed the title [DO NOT MERGE] SDK implementation review Refactored methods by each module Oct 11, 2020
@smc0210

smc0210 commented Oct 12, 2020

Copy link
Copy Markdown
Contributor

다음 이슈로 인해 header는 Authorization을 우선 적용하도록 수정된 이력이 있어서 common.py 파일의 헤더를 가져오는 부분도 이참에 반영되면 좋을 것 같습니다.
iamport/iamport-rest-client#2

@psy2848048 psy2848048 changed the title Refactored methods by each module Refactored methods by each module & implemented subscribe module Oct 12, 2020
@psy2848048

Copy link
Copy Markdown
Contributor Author

@smc0210 92eaee0

@psy2848048 psy2848048 force-pushed the private/wenli branch 2 times, most recently from 8a3a9da to 86c39eb Compare October 14, 2020 08:40
@codecov

codecov Bot commented Oct 14, 2020

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (features/protobuf@6fa6160). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@                 Coverage Diff                  @@
##             features/protobuf      #40   +/-   ##
====================================================
  Coverage                     ?   89.62%           
====================================================
  Files                        ?       13           
  Lines                        ?      588           
  Branches                     ?       26           
====================================================
  Hits                         ?      527           
  Misses                       ?       61           
  Partials                     ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fa6160...c84d494. Read the comment docs.

@psy2848048 psy2848048 merged commit 9c2ebc9 into features/protobuf Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants