Skip to content

Conversation

@singleview-co-kr
Copy link

@singleview-co-kr singleview-co-kr commented Aug 19, 2021

devkingsejong님 안녕하세요.
여유있으실 때 검토해 주시길 부탁드립니다.

아래는 제가 작업하면서 메모해놓은 코드 보완 내역입니다.

delete_master_report_all 명령 혹은 get_master_report_list 명령과 같이 정상 통신해도
응답값에 json이 포함되지 않아서 예외 발생해서 오류로 오인하게 하는 문제 개선

네이버 json 반환값이 list인 경우를 무시하고 dict만 반환한다는 전제 확장

BI 도구가 통신 결과를 정확히 이해하기 위해서 r.status_code == 400 이어서 r.ok == false인
경우에도 text의 code와 message 전달하도록 개선

BI 친화적이기 위해서 transation_id를 항상 처리하도록 개선

master stat 보고서 다운로드 기능 추가

CreateMasterReportObject의 fromtime 인수를 API가 허용하는 형식으로 전처리 코드 추가

계정정보 노출 주석 삭제delete_master_report_all 명령 혹은 get_master_report_list 명령과 같이 정상 통신해도
응답값에 json이 포함되지 않아서 예외 발생해서 오류로 오인하게 하는 문제 개선

네이버 json 반환값이 list인 경우를 무시하고 dict만 반환한다는 전제 확장

BI 도구가 통신 결과를 정확히 이해하기 위해서 r.status_code == 400 이어서 r.ok == false인
경우에도 text의 code와 message 전달하도록 개선

BI 친화적이기 위해서 transation_id를 항상 처리하도록 개선

master stat 보고서 다운로드 기능 추가

CreateMasterReportObject의 fromtime 인수를 API가 허용하는 형식으로 전처리 코드 추가

계정정보 노출 주석 삭제

delete_master_report_all 명령 혹은 get_master_report_list 명령과 같이 정상 통신해도
응답값에 json이 포함되지 않아서 예외 발생해서 오류로 오인하게 하는 문제 개선

네이버 json 반환값이 list인 경우를 무시하고 dict만 반환한다는 전제 확장

BI 도구가 통신 결과를 정확히 이해하기 위해서 r.status_code == 400 이어서 r.ok == false인
경우에도 text의 code와 message 전달하도록 개선

BI 친화적이기 위해서 transation_id를 항상 처리하도록 개선

master stat 보고서 다운로드 기능 추가

CreateMasterReportObject의 fromtime 인수를 API가 허용하는 형식으로 전처리 코드 추가

계정정보 노출 주석 삭제
@singleview-co-kr
Copy link
Author

뜻하지 않게 도움을 받은 점에 감사의 뜻을 전하고 싶어서
제가 활용하기 위해 보완한 코드를 PR 드립니다.

의아하실 수 있어서 메일 등으로 먼저 연락드리려고 노력했지만
제가 devkingsejong님의 연락처를 집요하게 찾아내는 것이 더 의아하실 수 있고

저도 사실은 집요하게 찾아낼 여유는 없어서 그냥 PR 메세지로
자세히 말씀드리기로 결정했습니다.

저에게 중요한 네이버 광고 운영 리포트를 가져오는 기능은 개선의 여지가 많았지만
깔끔한 구조 만으로도 제 고민의 90%는 덜어 주셔서 감사합니다.

저의 건의를 열린 마음으로 검토하시고 반영해 주신다면
추후에 저도 python-PowerNad을 명시적인 pip로 설치하고
사용할 수 있어서 더욱 좋을 것 같습니다.

하지만
당분간은 저의 필요에 의해서 더 추가해야 하는 코드가 있을지도 몰라서
제가 개발 중인 프로젝트의 라이브러리로 두는 점은
너그럽게 양해해 주시길 한번 더 부탁드립니다.

저의 필요에 의해서 더 추가해야 하는 코드가 있다면
신중히 검토하고 다시 PR 드리겠습니다.

관련된 코드 리포 주소는 아래와 같습니다.

sv_API_retriever

저는 온라인 광고 성과 분석과 평가를 위한 Biz Intelligence를 직접 개발하고 운영 중인데
(블로그를 잠깐 봤는데, 클라우드도 관심이 많으신 것 같습니다.
하지만 클라우드 BI는 빚좋은 개살구라는 점은 쉽게 공감하실 것도 같습니다.)

3~4년 전에 시간이 없어서 CLI로 대충 개발해 놓은 상태를
브라우저 기반의 GUI로 개선하고 싶었지만

네이버 광고 API를 django로 끌어오기 위해서 python으로
포팅된 API가 필요했지만 직접 포팅하기 막막해서 3~4년을 미루고 있었습니다.

3~4년간 네이버에서 제공한 PHP API를 꼼수로 호출하고 있었습니다.
당장 작동에 문제는 없었지만 php와 지저분하게 얽혀서
브라우저 베이스의 GUI로 건너가기 매우 망설여지는 고민을
devkingsejong님께서 도와주셨습니다.

당연히 문제가 없을 상황이었지만
그래도 혹시 몰라서 제 로컬 리포에서
master 브랜치와 merge도 확인했습니다.

제가 깃헙은 아직 낯설어서 혹시 PR 중에 무례한 점이 보인다면
잘 몰라서 그런 거라고 양해해 주시고
냉정하게 지적해 주시길 부탁드립니다.

@devkingsejong
Copy link
Owner

안녕하세요. 좋은 말씀과 좋은 업데이트 감사합니다. ^^
추가된 의존성을 requirements.txt에 추가해주시고, 가능하시다면 소스코드 마지막줄에 공백줄 추가 부탁드립니다.

제가 요즘 다른일을 하고 있어 병합에는 시간이 소요될 것 같습니다.
만약 빠른 병합을 원하시면 변경된 부분과 영향을 받는 코드들에 대한 호출 성공 여부 확인을 자동화 해주는 테스트 코드를 작성해주시면 확인후 즉시 병합해 드리겠습니다.(병합 필수사항은 아닙니다.)

좋은 하루 보내세요!

@singleview-co-kr
Copy link
Author

singleview-co-kr commented Aug 19, 2021

확인해 주셔서 감사드립니다.

  1. 제 생각에는 requirements.txt에 변동 사항은 없습니다.
    추가된 모듈은 restapi.py에 download 기능을 구현하기 위해 import shutil 뿐입니다.

  2. 사실은 마지막 공백줄이 저도 혼란스러웠지만
    제가 함부로 손댈 문제가 아닐 수 있어서 원래 파일 상태를 유지했습니다.

제가 헷갈렸을 수도 있어서. devkingsejong님의 마스터 최종 브랜치에서 한번 더 확인했습니다.

어찌되었던 제가 개입한 6개 소스에는 마지막 공백줄을 확실히 추가하고
이왕 하는 김에 전체 소스의 마지막 줄을 확인해서 다시 PR 드릴 수 있습니다.

이 부분 의견 부탁드립니다.

  1. 제가 보완한 기능의 테스트 코드 작성은 가능한데 아래의 문제를 확인 부탁드립니다.
    제가 보완한 기능은 모두 read feature인데
    테스트 코드를 작성한다 해도
    코드를 실행하는 대응 계정에 대응하는 보고서가 생성될
    운영 데이터가 충분해야 한다는 조건이 필요합니다.

이 부분 의견 부탁드립니다.

만약 보유하신 네이버 광고주 계정이 이 상태를 충족하지 못한다면
테스트 코드가 아닌 다른 방법을 통해서라도 오류 없음을 증명하여
devkingsejong님께서 걱정없이 병합할 수 있도록 도와야 할 책임은
저에게 있다고 생각합니다.

거짓없는 결과만 말씀드리면
이미 말씀드렸듯이 제가 자체 개발한 BI 도구를 약 4년간 상용으로 운영했고
python-PowerNad를 import한 후 전과 후의 읽어온 보고서 데이터가 동일함을
저를 위해서 수 차례 확인 했습니다.

이것은 저의 주장일 뿐이라는 점도 잘 알고 있습니다.

  1. 짐작하시듯이 병합은 저도 급한 문제가 아닙니다.

저도 기존 BI를 브라우저 베이스 GUI로 전환하는 과정에서
python-PowerNad를 추가로 손댈 가능성이 없다고 확신할 수 없고

그와 무관하게 여러가지 사항을 세심하게 검토하여
불필요한 위험없이 병합하시는 것이
서로에게 더 좋은 방법이라는 점을 충분히 공감합니다.

말씀해 주신 의견을 검토하여
제가 할 수 있는 부분에서는 성의 있게 기여하겠습니다.

깃헙 에디터가 익숙치 않아서 포맷이 오락가락하는 점은
너그럽게 양해 부탁드립니다.

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.

2 participants