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

removed pattern dependency when it is not needed in order to fix #461 #528

Merged
merged 8 commits into from
Nov 15, 2015

Conversation

ziky90
Copy link
Contributor

@ziky90 ziky90 commented Nov 13, 2015

This PR should fix the bug described in #461

return pattern


if has_pattern():
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, do we even need this if? When users try to call lemmatize, and pattern is not installed, they'll get a clear, standard ImportError.

I'm not sure why this if is here, I don't think it belongs any more, conceptually. But maybe I'm missing something.

@piskvorky
Copy link
Owner

Thanks @ziky90 ! It's a welcome change.

Not sure why we even need to mask out lemmatize() -- I think it can stay in utils, no matter whether pattern is installed or not.

@ziky90
Copy link
Contributor Author

ziky90 commented Nov 14, 2015

@piskvorky You're right. I've removed the if and added there check inside lemmatize() with hopefully clear message about that there needs to be pattern installed in order to use it.

Also the non passing test seems to me as not related to changes that I have made.

@piskvorky
Copy link
Owner

Thanks @ziky90 .

@gojomo this is the "rank of word2vec/doc2vec" failing test rearing its head again. If there's no way to make the tests more robust, I'm +1 on removing them, rather than obscure unrelated PRs.

@piskvorky
Copy link
Owner

@ziky90 before merging, can you also add a note to CHANGELOG summarizing this change?

And also mentioning how to deal with the missing HAS_PATTERN, in case people depended on that attribute, so they know how to migrate.

@ziky90
Copy link
Contributor Author

ziky90 commented Nov 14, 2015

Information about this PR added to the CHANGELOG.

@@ -1,6 +1,9 @@
Changes
=======

* Loading of pattern library in utils.py is only in lemmatize function (Jan Zikes, #461)
- utlis.HAS_PATTERN, has also chnged to utils.has_pattern()
Copy link
Owner

Choose a reason for hiding this comment

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

utlis => utils, chnged => changed :)

@ziky90
Copy link
Contributor Author

ziky90 commented Nov 14, 2015

I'm sorry for silly typos :(, anyway typos were fixed.

@piskvorky
Copy link
Owner

Perfect, merging. Thanks @ziky90

piskvorky added a commit that referenced this pull request Nov 15, 2015
removed pattern dependency when it is not needed in order to fix #461
@piskvorky piskvorky merged commit 463b94d into piskvorky:develop Nov 15, 2015
@ziky90 ziky90 deleted the fix_461 branch November 24, 2015 13:01
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.

None yet

2 participants