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

Issue #929 - Generic error handler based on status code for status 40… #949

Closed
wants to merge 3 commits into from
Closed

Conversation

deepthivenkat
Copy link
Member

…0,401,403,404

@miketaylr
Copy link
Member

r? @karlcow

(I can review too, but Karl suggested this approach -- would be interested in his feedback).

@miketaylr
Copy link
Member

@deepthivenkat can you run pep8 and fix up any lint errors there?

The command "pep8 --ignore=E402 webcompat/ tests/ config/secrets.py.example" failed and exited with 1 during .

@deepthivenkat
Copy link
Member Author

Sure..checking
On Mar 7, 2016 11:43 PM, "Mike Taylor" [email protected] wrote:

@deepthivenkat https://github.com/deepthivenkat can you run pep8 and
fix up any lint errors there?

The command "pep8 --ignore=E402 webcompat/ tests/
config/secrets.py.example" failed and exited with 1 during .


Reply to this email directly or view it on GitHub
#949 (comment)
.

@karlcow
Copy link
Member

karlcow commented Mar 8, 2016

@deepthivenkat Thanks for starting working on this.

Let's discuss a bit more. ^_^

So in my approach I realized that I mixed two ways of making it more DRY, and it's probably over-doing it.
There's room for discussions on how to improve in a step by step.

  • If we are sure that all 4** HTTP codes follow the same pattern. We do not to abstract the two functions (api_call(request), api_message(code)). We just need to have the appropriate decorators on top of one generic function.
  • If we have different treatments in between the 4** HTTP codes then the abstraction of the two methods is worth.

We still need a dictionary for error messages. We also need to figure out if we want to have separate messaging for HTML (Human calls) and JSON (API calls).

Opinions? If what I say above is not clear enough, I can go a bit more in details. :)

@karlcow
Copy link
Member

karlcow commented Mar 11, 2016

There's a need to clean this pull request with the input of #958 and also it removes some data. etc.
Maybe easier would be to restart with a clean branch and delete this pull request and the #958
If you need guidance tell us.

@deepthivenkat
Copy link
Member Author

I accidentally deleted requirements.txt. there were few more issues. So I deleted the repository and cloned it again and started with a clean branch(the one I submitted yesterday is from this new branch). Is it okay if we continue with the new pull request?

@magsout
Copy link
Member

magsout commented Mar 11, 2016

@deepthivenkat so let's closed this issue in favor of #958, sounds good ?

@deepthivenkat
Copy link
Member Author

Yes..Thanks :)

@magsout magsout closed this Mar 11, 2016
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.

4 participants