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

Twitter Package update #156

Merged
merged 5 commits into from Jul 31, 2018

Conversation

Projects
None yet
4 participants
@zsef123
Copy link
Contributor

zsef123 commented Aug 3, 2017

morphs(phrase, norm=False, stem=False)
Parse phrase to morphemes.

document와 parameter 에서는 norm과 stem 인자를 받는것으로 나와있는데
morphs에서 pos를 호출할때 인자가 빠져있어서 수정했습니다.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 3, 2017

Coverage Status

Coverage remained the same at 56.429% when pulling 7096c33 on zsef123:master into d0fd8e8 on konlpy:master.

1 similar comment
@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 3, 2017

Coverage Status

Coverage remained the same at 56.429% when pulling 7096c33 on zsef123:master into d0fd8e8 on konlpy:master.

@zsef123 zsef123 force-pushed the zsef123:master branch from bc11263 to 7096c33 Aug 5, 2017

@zsef123

This comment has been minimized.

Copy link
Contributor

zsef123 commented Aug 6, 2017

저번에 했던 Pull Request에 이어서 달겠습니다.

Change twitter-korean-text dependencies to open-korean-text #141

Twitter Korean Text 가 Open Korean Text로 이름을 바꾸어 업데이트를 하신다고 합니다.
따라서 기존의 Twitter package를 현재 최신 버전의 Open Korean Text로 적용시켰습니다.

Open-Korean-Text , scala-library, twitter-text jar 버전을 현재 OKT에 맞게 업데이트 했으며
test case와 주석부분의 예시 또한 현재 패키지에 맞게끔 바꿨습니다.

@zsef123 zsef123 changed the title Twitter Class의 morphs 수정 Twitter Package update Aug 6, 2017

@e9t

This comment has been minimized.

Copy link
Member

e9t commented Sep 3, 2017

@zsef123 PR 감사합니다!!! 👍
이 PR로 크게 세 가지 이슈를 제기해주셨네요. 그 중 가장 중요한 두 가지에 대해 의견 드리겠습니다.

  1. Twitter morphs에서 stem, norm이 미적용된 버그 fix: 이 commit은 cherry pick해서 다음 마이너 버전인 0.4.5에 적용될 수 있게 하겠습니다.
  2. Tkt를 okt로 변경: 클래스의 이름이 바뀌면 기존에 konlpy를 사용하던 유저들의 코드가 깨지게 생기게 됩니다. 가장 좋다고 생각되는 방법은 다음 마이너 버전업에서 일단 Twitter class의 현재 행동을 유지하되 deprecation warning을 띄우고, 차후 1.0.0 으로 버전업할 때 api를 완전히 replace하는 방법입니다. 이 의견에 동의하신다면 Twitter를 호출할 경우 Okt로 redirection해주고, 동시에 deprecation warning을 추가해주실 수 있나요?

그 외에도 Travis에서 test fail 되고 있는 문제들도 fix를 해야할 것 같은데, 이 부분은 저도 다시 확인해보겠습니다.

@e9t e9t self-requested a review Sep 3, 2017

@zsef123

This comment has been minimized.

Copy link
Contributor

zsef123 commented Sep 3, 2017

@e9t 현재 tag directory의 Twitter Class의 method와 이름은 그대로 두고 내부의 Java Interface와 Class만 변경을 시켜서 기존 코드와 문제가 발생하지는 않습니다.
물론 내부 메커니즘이 바뀌어서 기존의 결과와 동일하지는 않습니다만, Twitter 사용자들은 기존의 코드를 그대로 사용 가능합니다.

향 후 Twitter의 이름을 Okt에 맞게 바꾸실 계획도 있으시다면 Deprecation하는게 맞는것 같습니다.

제안 주신 것과 Travis failed도 다시 살펴서 커밋 올리겠습니다.

@zsef123 zsef123 force-pushed the zsef123:master branch 2 times, most recently from abab0cf to c473b9c Sep 4, 2017

>>> print(twitter.phrases(u'날카로운 분석과 신뢰감 있는 진행으로'))
['분석', '분석과 신뢰감', '신뢰감', '분석과 신뢰감 있는 진행', '신뢰감 있는 진행', '진행', '신뢰']
>>> print(twitter.pos(u'이것도 되나욬ㅋㅋ'))
>>> okt = Okt()

This comment has been minimized.

@Kcrong

Kcrong Jan 19, 2018

@zsef123 주석에 임포트 코드 하나가 누락된 것 같습니다.

from konlpy.tag import Okt

@e9t e9t added this to the v0.4.5 milestone Jul 30, 2018

@e9t e9t merged commit c473b9c into konlpy:master Jul 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment