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

Error handling #1

Merged
merged 5 commits into from
Dec 26, 2018
Merged

Error handling #1

merged 5 commits into from
Dec 26, 2018

Conversation

shijuleon
Copy link
Contributor

HTTP requests error handling

Also adding command line parameters

@sdushantha
Copy link
Member

Hi @shijuleon,

Could you add some styling to the HTTP requests errors? What I mean with styling is color, just like the other outputs.
I like the idea of adding command line parameters, but it would be much better if you could add a help message. So, if the user adds a --help flag, they would get the help message.

Other than that, everything seems great!

This was referenced Dec 25, 2018
@shijuleon
Copy link
Contributor Author

Sure! I have added some quick changes. You can improve it if you wish.

@sdushantha
Copy link
Member

Could fix two things?
1. At the moment, if the user does not provide a username, it shows the help message, but in the code, you have set it so that, if the username is not provided as an argument, then the user will be prompted.

2. Could you do something with the error message? Its kinda messy. Instead of showing the error, could you show the name of the social media instead?

[-] Error Connecting: AngleList

The error in the image is related to issue #9
screenshot_18-12-26_at_09-53-38

@shijuleon
Copy link
Contributor Author

Fixed the error printing.

I don't think we need the option to enter username. As a command line parameter seems fine. What do you think?

@sdushantha
Copy link
Member

Could you remove the banner from the help message? In my opinion, it only looks good when the script is actually running not when just showing the help message.

If you fix that, I will merge it.

@shijuleon
Copy link
Contributor Author

I have removed the banner. Just thought it looked fancy.

@sdushantha
Copy link
Member

Thank you! 😄

@sdushantha sdushantha merged commit 7a55917 into sherlock-project:master Dec 26, 2018
TheYahya pushed a commit that referenced this pull request Jan 17, 2019
hoadlck pushed a commit that referenced this pull request Sep 24, 2019
sdushantha pushed a commit that referenced this pull request Jul 27, 2020
sdushantha pushed a commit that referenced this pull request Oct 27, 2021
sdushantha pushed a commit that referenced this pull request Sep 23, 2022
sdushantha pushed a commit that referenced this pull request Dec 28, 2022
added browse functionality to notify.py
johnneijzen referenced this pull request in johnneijzen/sherlock Mar 7, 2024
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