TestApp.delete causes spurious warning about webtest HOT 8 CLOSED

Basically adding correct default to all methods would fix this. With a test case that warning is raised and adding stacklevel param to the warning.

from webtest.

I could make a PR if you’re patient with my lack of git-fu.

from webtest.

Sure, how can I help? :)

from webtest.

I know how to clone, commit and push, but merging is still infuriatingly confusing to me, so if I use the wrong branch or get an unmergeable PR, I’ll need help :)

from webtest.

Workflow is the following:

• you fork this repo
• git clone your fork
• commit changes
• push back to your repo
• on github, click "pull request", review changes and submit it

On Mon, Mar 4, 2013 at 6:40 AM, Éric Araujo [email protected]:

I know how to clone, commit and push, but merging is still infuriatingly
confusing to me, so if I use the wrong branch or get an unmergeable PR,
I’ll need help :)

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/50#issuecomment-14364792
.

from webtest.

Yep, I know the basic flow as I said in my previous message, it’s really the branching model and the synchronization that don’t fit my head yet.

Basically adding correct default to all methods would fix this.

I did that and it does remove the spurious warning for DELETE.

With a test case that warning is raised and adding stacklevel param to the warning.

Problem is that warnings use their own reporting system, so you can’t just use assertRaises. In the stdlib we have special helpers to check that a warning was sent, but there’s nothing to do that in webtest at present.

stacklevel=3 does the right thing (3 = _gen_request → delete method → caller). I wanted to separate the two changes in two commits and then git hated me so I won’t push anything today, but I wanted to confirm the fixes.

from webtest.

I think the correct way is to fix the warning issuing logic. Why is checking for if method == 'DELETE' and params: not enough? How is u'' non-empty?

from webtest.

Thanks for the fix @noonat ! The stacklevel value is still the old one, I may open another ticket for that.

from webtest.