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

refactor: DRY version #75

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

refactor: DRY version #75

wants to merge 1 commit into from

Conversation

wazaundtechnik
Copy link
Contributor

Do not hard code version into commands. Put it into separate file and let another module calls that command to retrieve version information.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.1%) to 93.493% when pulling accbf7e on wazaundtechnik:feature/standalone-version into 4b8f011 on tulul:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.1%) to 93.493% when pulling 472dd13 on wazaundtechnik:feature/standalone-version into 4b8f011 on tulul:master.

@@ -55,11 +55,12 @@ def quote(message):
def who(message):
app.logger.debug('Detected who command {!r}'.format(message.text))
about_text = (
'TululBot v1.11.2\n\n'
'TululBot {!r}\n\n'
Copy link

Choose a reason for hiding this comment

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

The !r is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@@ -110,11 +112,12 @@ def test_who(fake_message, mocker):
who(fake_message)

expected_text = (
'TululBot v1.11.2\n\n'
'TululBot {!r}\n\n'
Copy link

Choose a reason for hiding this comment

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

The !r is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

VERSION = '1.11.3'


def version_formatted():
Copy link

Choose a reason for hiding this comment

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

Since you can import VERSION directly, this function is not really needed isn't it? At this point, the function only preprends v to the version number. Also, this variable might be best put under tululbot/__init__.py file so that it can be imported like from tululbot import VERSION.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know we can import a constant. Thanks

@wazaundtechnik
Copy link
Contributor Author

wazaundtechnik commented Jul 23, 2017

Hey team i tried to change to what @kmkurn suggest, but the build was failed https://travis-ci.org/tulul/tululbot/builds/256555060. Can somebody help me and point out where is the mistake?

Previous build worked (when use standalone VERSION file).

@kmkurn
Copy link

kmkurn commented Jul 23, 2017

@wazaundtechnik Sorry I think that's probably because of circular import or sth. I'm not sure either because I don't check myself. What if you put VERSION under tululbot/constants.py file, for instance?

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