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

default_block/CreateNewBlock,default_block_test/TestCreateN… #12

Closed
wants to merge 5 commits into from
Closed

default_block/CreateNewBlock,default_block_test/TestCreateN… #12

wants to merge 5 commits into from

Conversation

ttkmw
Copy link
Collaborator

@ttkmw ttkmw commented Apr 20, 2018

default_block의 CreateNewBlock과 테스트 케이스를 작성해봤습니다.

CreateNewBlock 코드가 머지된 후에는 json/yaml파일을 토대로 CreateGenesisBlock을 작성할 계획입니다.

피드백 부탁드립니다!

@ttkmw ttkmw closed this Apr 20, 2018
@ttkmw ttkmw reopened this Apr 20, 2018
@ttkmw
Copy link
Collaborator Author

ttkmw commented Apr 20, 2018

왜인지 리뷰어 지정이 안뜨네요...ㅠ

@ttkmw ttkmw changed the title junksound:default_block/CreateNewBlock,default_block_test/TestCreateN… default_block/CreateNewBlock,default_block_test/TestCreateN… Apr 20, 2018
@byron1st
Copy link
Contributor

제가 리뷰어 지정 했습니다.

Copy link
Contributor

@byron1st byron1st left a comment

Choose a reason for hiding this comment

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

코드 포멧팅 관련해서 아래 링크 사항이 반영된 것인지 검토 부탁드려요. 괄호 위치나 임포트 형태로 볼 때 아래 링크의 포멧팅이 적용되지 않은 것 같습니다. 그럴경우, 제 풀리퀘와 default_block.go 파일에서 충돌 날 수 있을 것 같네요.

코드 포멧팅 제안 #108

@ttkmw
Copy link
Collaborator Author

ttkmw commented Apr 21, 2018

수정하겠습니다!

@ttkmw ttkmw closed this Apr 21, 2018
@ttkmw ttkmw reopened this Apr 21, 2018
@ttkmw
Copy link
Collaborator Author

ttkmw commented Apr 21, 2018

파일와쳐를 쓰긴 했는데...잘 된건지 모르겠네요.
커맨드+S로 새로 저장하면 임포트 위치가 바뀌긴 하는데,
그게 항상 일정한 게 아니고 라이브러리 임포트 순서에 따라 달라지는 걸 보니 좀 문제가 있는거같습니다ㅠ
예를 들어,
import (time, fmt, sort, error)일 때는
time, sort, fmt, error로 바뀌는데,

import (time, fmt, error, sort)일 때는
sort, time, fmt, error 이런식으로

기존 import 순서에 따라 결과가 달라집니다.
정상인가요..?
@byron1st
조언 부탁드립니다..!ㅠ

@byron1st
Copy link
Contributor

@junk-sound
음, 그 부분은 좀 이상하네요. 일단 goimports를 적용하신 것이면 일단 진행하고, merge 할 때 혹시 conflict가 나면 수정하는게 좋을 것 같아요.

@byron1st
Copy link
Contributor

엄.. merge conflict 나길래 제가 resolve 했는데, resolve 하니까 그냥 develop 브랜치를 feature/block 브랜치로 머지시키네요 ㅎㅎ

@ttkmw
Copy link
Collaborator Author

ttkmw commented Apr 21, 2018

@byron1st

  1. 넵 알겠습니다 goimports가 약간 제멋대로인 부분이 있는 것 같은데, 컨플릭트 안뜨도록 잘 정렬하겠습니다~
  2. 오잉 그래도 괜찮은거겠죠...? feature/block브랜치가 develop 브랜치에서 시작된걸텐데 왜인지 모르겠네요... 한번 확인해보고 이유를 알게되면 코멘트 달겠습니다~

@ttkmw
Copy link
Collaborator Author

ttkmw commented Apr 21, 2018

@byron1st @junbeomlee
<마지막 커밋의 주 내용은 다음과 같습니다.>

  1. CreateGenesisBlock
  2. ConfigFromJson
    2는 genesisblock의 메타정보를 불러들어오고, 1은 2를 활용해 GenesisBlock을 만듭니다.

<꺼름칙한 부분은 다음과 같습니다.>

  1. ConfigFromJson에서 제이슨 파일을 상대경로로 가져옴.
  2. ConfigFromJson에서 불러온 제이슨 파일을 바이트로 바꿀 때, End of File이라는 에러가 발생할 수도 있다고 하는데, 현재까지는 이 에러가 언제 발생하는지 잘 모르고 있습니다. 그래서 테스트할 때 뺐습니다. 여기서 설령 에러가 난다고 하더라도 CreateGenesisBlock에서 에러를 알 수 있기 때문에 큰 문제는 없을 것 같은데, 문제가 된다면 End of File 에러가 언제 발생하는 지 더 찾아보겠습니다.
  3. TestCreateGenesisBlock할 때, 블럭이 생성된 시간과 검사하는 시간이 아주 조금 차이나기 때문에 시간, 분, 초까지만 맞으면 괜찮다고 해놨습니다.

피드백 부탁드립니다!

p.s: 이전 커밋이 컨플릭트 떴던 것은 제가 풀을 안했어서 그런 것 같습니다.

@byron1st byron1st mentioned this pull request Apr 23, 2018
@byron1st
Copy link
Contributor

issue #9 에서 논의한대로, 일단 CreateGenesisBlockConfigFromJson 함수 및 관련 테스트를 코멘트 처리하거나 제거한 후 다시 푸쉬해주시면 제가 머지 하도록 하겠습니다.
위의 함수들은 아마 it-chain-Engineblockchain/api 로 가야 할 것 같은데, 해당부분은 @junbeomlee 님이 한번 검토해주셔요.

@junbeomlee
Copy link
Member

커밋된 코드들은 it-chain-Engine으로 옮기는 것으로 하고 close하겠습니다.

@junbeomlee junbeomlee closed this Apr 25, 2018
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