Skip to content
This repository has been archived by the owner on Nov 29, 2022. It is now read-only.

add cron master util #8

Merged
merged 2 commits into from Oct 13, 2017
Merged

Conversation

kobi97
Copy link
Contributor

@kobi97 kobi97 commented Sep 27, 2017

cron master를 util화 하기 위한 첫단계로 platform-common으로 이동하도록 이야기가 진행되어 리뷰 완료 후 PR 올립니다.

Copy link
Contributor

@genesos genesos left a comment

Choose a reason for hiding this comment

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

composer 참조 확인 필요

namespace Ridibooks\Platform\Common\Cron;

use Exception;
use Ridibooks\Exception\MsgException;
Copy link
Contributor

Choose a reason for hiding this comment

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

@ssaifriend
지금보니 MsgException 이거 "ridibooks/store"의 MsgException를 참고하는것 같은데,
지금 composer 참조상으로는 없네요. 어떻게 하면 좋을까용?

Copy link
Contributor Author

@kobi97 kobi97 Oct 10, 2017

Choose a reason for hiding this comment

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

@genesos @ssaifriend MsgException이 core에 있는게 아니고, store에 있었네요 -_-;; Ridibooks\Platform\Common\Exception\CommonException or MsgException 을 하나 추가하는 것은 어떠신가요?

Copy link
Contributor

Choose a reason for hiding this comment

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

넵 전 그게 좋을것 같습니다!!
해당방법이 선택되면 다른 MsgException도 플랫폼팀걸 쓰도록 하시지용 ㅎㅎ

Copy link
Contributor

Choose a reason for hiding this comment

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

저도 동의합니다~!

Copy link
Contributor Author

@kobi97 kobi97 Oct 12, 2017

Choose a reason for hiding this comment

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

@genesos @ssaifriend 코드를 쭈욱 확인해보았는데요. 이미 다른 Utils들에서 store의 MsgException들을 사용하고 있고, JsonDto 같은 경우 store의 MsgException에 의존성이 있어서, 새롭게 만들면 2개의 MsgException을 처리해야 합니다. 그 외에도 추후 store팀에서 플랫폼팀 코드를 사용하게 되면 MsgException이 2개라서 문제의 소지가 있어 보입니다. ssaifriend과 이야기해보았는데, store에 요청해서 MsgException을 core에 옮기는 요청을 보내기로 결론 내렸습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

따라서 해당 merge는 이대로 진행해도 될 것 같습니다.

@namenu
Copy link
Contributor

namenu commented Oct 12, 2017

@kobi97
Copy link
Contributor Author

kobi97 commented Oct 12, 2017

@namenu 넵 참고해서 적용하겠습니다.

@genesos genesos merged commit 7d4dce7 into ridi:master Oct 13, 2017
@kobi97 kobi97 deleted the feature/cron_master_util branch October 13, 2017 03:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants