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

labhub.py: Add error message when nick is empty #642

Merged
merged 1 commit into from
Nov 6, 2018

Conversation

abhishalya
Copy link
Member

@abhishalya abhishalya commented Nov 2, 2018

All of the backends do not have nick attribute.
Hence an error will be shown whenever nick is empty.

Closes #632

Reviewers Checklist

  • Appropriate logging is done.
  • Appropriate error responses.
  • Handle every possible exception.
  • Make sure there is a docstring in the command functions. Hint: Lookout for
    botcmd and re_botcmd decorators.
  • See that 100% coverage is there.
  • See to it that mocking is not done where it is not necessary.

@TravisBuddy
Copy link

Hey @abhishalya,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 3756e790-ded6-11e8-9517-3f3b7d8cc1c8

plugins/labhub.py Outdated Show resolved Hide resolved
plugins/labhub.py Outdated Show resolved Hide resolved
plugins/labhub.py Outdated Show resolved Hide resolved
plugins/labhub.py Outdated Show resolved Hide resolved
plugins/labhub.py Outdated Show resolved Hide resolved
plugins/labhub.py Outdated Show resolved Hide resolved
@abhishalya
Copy link
Member Author

I apologize for not checking the code locally. WIll not push until checked :)

Copy link
Member

@nvzard nvzard left a comment

Choose a reason for hiding this comment

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

You'll also need to write tests along with cleaning up the code :)

@abhishalya
Copy link
Member Author

@nvzard Sure, is this the correct approach though? Do I need to make any changes in the statements?

plugins/labhub.py Outdated Show resolved Hide resolved
plugins/labhub.py Outdated Show resolved Hide resolved
plugins/labhub.py Outdated Show resolved Hide resolved
@abhishalya
Copy link
Member Author

Ping @nvzard @meetmangukiya

tests/labhub_test.py Outdated Show resolved Hide resolved
tests/labhub_test.py Outdated Show resolved Hide resolved
@abhishalya
Copy link
Member Author

abhishalya commented Nov 6, 2018

Btw InvalidLinkBear is complaining:

plugins/ship_it.py
|  14| ········'http://i.imgur.com/DPVM1.png',
|    | [NORMAL] InvalidLinkBear:
|    | Broken link - unable to connect to http://i.imgur.com/DPVM1.png (HTTP Error: 404)

Copy link
Member

@meetmangukiya meetmangukiya left a comment

Choose a reason for hiding this comment

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

We have to test if the error messages are being sent whenever the nick is not available. Create a new test function test_empty_nick where you create a message that has an empty nick, and make errbot use that mocked empty nick(am not sure how you can do this. Maybe @nvzard knows?).

Also, you are involved with both @jayvdb and @nvzard, so if they suggested something different, go with that.

plugins/labhub.py Outdated Show resolved Hide resolved
tests/labhub_test.py Outdated Show resolved Hide resolved
tests/labhub_test.py Outdated Show resolved Hide resolved
tests/labhub_test.py Outdated Show resolved Hide resolved
tests/labhub_test.py Outdated Show resolved Hide resolved
@abhishalya abhishalya force-pushed the nickEmptyError branch 2 times, most recently from 1125087 to 9b2e2c7 Compare November 6, 2018 09:45
plugins/labhub.py Show resolved Hide resolved
tests/labhub_test.py Outdated Show resolved Hide resolved
All of the backends do not have `nick` attribute.
Hence an error will be shown whenever nick is empty.

Closes coala#632
Copy link
Member

@nvzard nvzard left a comment

Choose a reason for hiding this comment

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

Good Work 👍

@jayvdb
Copy link
Member

jayvdb commented Nov 6, 2018

ack 4c61393

@jayvdb
Copy link
Member

jayvdb commented Nov 6, 2018

@gitmate-bot ff

@gitmate-bot
Copy link

Hey! I'm GitMate.io! This pull request is being fastforwarded automatically. Please DO NOT push while fastforward is in progress or your changes would be lost permanently ⚠️

@gitmate-bot
Copy link

Automated fastforward with GitMate.io was successful! 🎉

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

Successfully merging this pull request may close these issues.

6 participants