-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
added test for sync_state #2959
Conversation
gensim/test/test_ldamodel.py
Outdated
@@ -42,6 +43,25 @@ def setUp(self): | |||
self.class_ = ldamodel.LdaModel | |||
self.model = self.class_(corpus, id2word=dictionary, num_topics=2, passes=100) | |||
|
|||
def testSyncState(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a question for @piskvorky / @mpenkov - our existing unit-test methods are a mix of camelCase
and snake_case
- somewhat owing to the way Python's unittest
module names were largely imported from Java, and thus Python conventions around unit-testing classes/methods have been a mix. But, we should probably strongly prefer one or the other for new methods, and gradually bring old methods into conformance. I prefer snake_case
, with the only exception being methods that specifically override a unittest
camelCase
precedent - so that all methods we ourselves write follow the single Python convention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly right. I also prefer snake_case
, some code has camelCase
due to Python's unittest
historic reasons only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation has it the way gojomo prefers: https://docs.python.org/3/library/unittest.html setUp
and test_*
I can rename them at some point, but first I'd like to check if the documentation for #2282 is complete, fix the merge conflict and regain push permissions on the branch since I left DataReply quite some itme ago.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, method names should be snake_case
everywhere except where it is unavoidable, like the infamous setUp
and friends.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(oh I guess I could/should have added that to this PR)
Merged. Thank you @sezanzeb ! |
No description provided.