search

Suor / handy

Handy django tools
BSD 3-Clause "New" or "Revised" License
85 stars 14 forks source link

Ajax stuff improvement #4

Closed caxap closed 12 years ago

caxap commented 12 years ago
Suor commented 12 years ago

What's the point of returning AjaxError instead of raising? catch() taking multiple exceptions look useful, but why take junk and then filter it out? Also, please don't rename e to exc.

caxap commented 12 years ago
  1. It's just syntacsys sugar. Also might in some places you don't want to know that exception has "params" property (or from other side the exception has json representation)
  2. Filtering it's additional (optional) validation step, so it might unnecessary here.
  3. "e" renamed to "exc" to avoid name hiding. Is it so critical to user "e" in try-except statement?
Suor commented 12 years ago
  1. It's a semantic change which permits returning ajax.error() instead of raising, for which I see no use. Even if there were some use, such a strange behavior as returning exceptions should be avoided not encouraged. Also, your code will result in {"success": true, ...} response upon AjaxError return which is very confusing.
  2. Passing anything but exception classes to catch() should raise errors.
  3. It's part of my code style, and I'd like too keep it throughout handy code. Anyway, there is no name clash there.

There are parts of Zen of Python about 1 and 2:

  1. There should be one obvious way to do it (exceptions should be raised not passed).
  2. Errors should never be silenced.

Giving all of that, the only thing from this patch that I see as useful and will willingly accept is multiple arguments for catch()

caxap commented 12 years ago

So, updated! I use slightly modified version of ajax decorator and in my case these changes make sense. I'm agree that in your case it looks redundant.

Anyway, there is no name clash there.

actually it was, name "e" was used in filtering and in except statements, and yes there are no any side effects from this hiding.

Suor commented 12 years ago

Can you, please, rebase your branch to make patch cleaner?

Also, if you don't mind can you share a code of your view or views that required things you submitted? This is not necessary, just curious. A gist would be perfect.

Suor commented 12 years ago

Merged manually in 3b1ee864da