search

funprog / funbot

MIT License
5 stars 2 forks source link

DiceBot 클래스를 추가 #18

Closed jwChung closed 8 years ago

jwChung commented 8 years ago

Dicebot 클래스를 추가합니다.

이 PR은 #5 이슈와 관련됩니다.

jwChung commented 8 years ago

@funprog/core 리뷰 부탁합니다.

younggi commented 8 years ago

dice를 roll 하는 core 기능을 만든다고 생각을 해보면, object name이 DiceBot보다 조금 하위 level의 이름이면 더 좋을 것 같다는 생각이 들지만, 어떤것이 좋을 지는 생각이 잘 안나네요.

jwChung commented 8 years ago

@younggi

조금 하위 level의 이름

이라고 표현하신게 더 구체적인 이름을 의미하시는 건가요?

younggi commented 8 years ago

slack의 api request를 처리하는 부분까지를 포함한 부분 DiceBot이라 생각이 들어서 의견 적어봤습니다. 하위 level이란 이름은 모호했네요..

작은 시작이라 큰 문제는 없겠지만, 전체 slack bot에서 layer가 구분되면 함께 작업하는데 좀 더 편할까..하는 생각에 적어봤습니다.

funbot 이니 다른 bot도 만들어 넣을 수 있으면 좋겠다는 생각도 들었네요...

younggi commented 8 years ago

말씀하신 좀 더 구체적인 이름일 수도 있는데, 일단 그냥 진행하셔도 됩니다.~ ^^

jwChung commented 8 years ago

@younggi 네 좋은 의견이십니다. 그런데 지금 상태에서는 디자인을 최대한 고려하지 않고 Dice Bot 구현에 대해서만 촛점을 맞추는게 어떨까요? 아마 @kwoolytech 님도 #14 에서 비슷한 고민을 하고 계시지 않나 생각됩니다.

hkjlee109 commented 8 years ago

Decent start.

younggi commented 8 years ago

넵~ 고고~ 하시죠^^

jwChung commented 8 years ago

@younggi 넵 감사합니다. :) @myeesan 님 리뷰의견이 궁금한데 듣고 진행하는 건 어떨까요?

myeesan commented 8 years ago

넵 방금 서버 테스트 완료했습니다. 우선 Request 부분 핸들링은 다음 오프 이후에 결정하는게 좋을 것 같습니다. 현재 Web hook 방식 외에 Command 방식도 있는데 어떤 방식을 사용할지 결정해야 하고요, Request를 직접 처리하려면 웹 프레임워크를 다뤄야 해서요. 우선은 dice bot 구현 부터 완료하고 진행해도 무리 없다고 생각합니다.

jwChung commented 8 years ago

다음 시간에 The Rules of the Open Road 내용 같이 한번 봤으면 좋겠네요. 아래는 이 문서의 일부를 발췌한 것입니다.

When you think the code is done, don’t merge it into master; instead, open a pull request from your working branch to master. This pull request will give your fellow contributors a place to review the code, and to discuss your work. Once the review is done and the discussion has ended, one of the reviewing contributors will merge the pull request. You should never merge your own pull request. Having another contributor merge the pull request does one better than just saying “Ship It!”.

몇몇 분들이 이 PR 승인 의견을 주셨습니다. merge해도 되겠다는 판단이드는데요. 그런데 제가 직접 merge를 하는 것은 위 내용과 같이 적절하지 않다고 생각되네요. 다른 분이 merge 해주시면 좋겠네요.

hkjlee109 commented 8 years ago

So, your intention on this PR was to create a skeleton of the dice class and the test code. Good job on processing everything so far! FYI, I am seeing this message, Only those with write access to this repository can merge pull requests.

hkjlee109 commented 8 years ago

I have updated the decription so that people may know the scope of this PR. Hope you don't mind. :)

jwChung commented 8 years ago

@kwoolytech

Only those with write access to this repository can merge pull requests.

I am going to address this issue. Thank you for pointing that out.

I have updated the decription so that people may know the scope of this PR. Hope you don't mind.

I must say 👍 "히트다 히트" :)