Add get_request_kwargs to check before requesting #339

kumarstack55 · 2020-10-14T13:17:16Z

When we use mechanicalsoup, we sometimes want to verify a request before submitting it.

If you merge this pull request, the package will be able to provide a way for the package's users to review the request.

This is my first pull request for this project. Please let me know if I'm missing anything.

hemberger

Hi, and thanks for your contribution to MechanicalSoup!

In principle, I think this is a fine change. I've recommended one small spelling error and identified a corner case that probably needs to be addressed. Once those are fixed, I will be ready to approve this. Then if the other maintainer approves as well we can merge it in. :)

mechanicalsoup/browser.py

…Soup#339

kumarstack55 · 2020-10-17T12:34:14Z

The group of jobs in travisci are listed as Queued, but when I click on Details to check, it looks like all the jobs are successful.

kumarstack55 · 2020-10-17T12:38:37Z

The group of jobs in travisci are listed as Queued, but when I click on Details to check, it looks like all the jobs are successful.

@hemberger Is there anything else I should do? :)

moy · 2020-10-17T19:32:42Z

Commits should be squashed and the typo should be fixed in the commits summary (I just fixed it in the PR title). We keep a clean history hence a PR should include several commits only if the change needs to be split into several commits, not as leftover of previous mistakes.

moy

On overall, this looks good, but a few more nits to be fixed before this can be merged.

moy · 2020-10-17T19:35:11Z

mechanicalsoup/browser.py

-    def _request(self, form, url=None, **kwargs):
-        """Extract input data from the form to pass to a Requests session."""
+    @classmethod
+    def _get_request_kwargs(cls, method, url, **kwargs):


cls is unused here, right? Why not use @staticmethod instead of @classmethod then?

Thanks for the good point. I'll use @staticmethod to avoid making cls useless.

moy · 2020-10-17T19:36:09Z

mechanicalsoup/browser.py

+    @classmethod
+    def _get_request_kwargs(cls, method, url, **kwargs):
+        # This method exists to raise a TypeError when a method or url is
+        # specified in the kwargs.


Shouldn't this comment be a docstring?

Actually, this is my first time writing a docstring.

I'll be referring to other methods to write docstrings. Please let me know if you have any concerns.

moy · 2020-10-17T19:46:51Z

tests/test_browser.py

+    try:
+        browser.get_request_kwargs(form, page.url, **kwargs)
+    except TypeError:
+        type_error_raised = True


Please use pytest.raises to assert that an exception is raised.

Thanks, I'll rewrite it using pytest.raises.

moy · 2020-10-17T19:47:19Z

tests/test_browser.py

+        # pylint: disable=redundant-keyword-arg
+        browser.get_request_kwargs(form, page.url, **kwargs)
+    except TypeError:
+        type_error_raised = True


Thanks, I'll rewrite it using pytest.raises.

kumarstack55 · 2020-10-18T06:04:49Z

Originally, this pull request was intended to implement _get_request_kwargs(), but the name was changed and get_request_kwargs() is the current name.

Therefore, please let me change the title of the Pull Request.

kumarstack55 · 2020-10-19T13:23:07Z

Commits should be squashed and the typo should be fixed in the commits summary (I just fixed it in the PR title).

I squashed the commits to one. ✨

moy · 2020-10-19T14:03:36Z

On overall this looks good to me. The commit message would deserve a bit more love I think (explain why the change is needed and good).

kumarstack55 · 2020-10-19T23:33:46Z

On overall this looks good to me. The commit message would deserve a bit more love I think (explain why the change is needed and good).

Let me make the commit log more understandable and lovely. I don't plan to make any changes to the code at this time.

kumarstack55 · 2020-10-31T13:33:46Z

@hemberger @moy Let me know if there's anything else I need to do to get the pull request merged.

hemberger

Overall, the changes look great. I've identified one simple change I'd like to see, but then I think this is ready to be merged. Thanks!

mechanicalsoup/browser.py

Previously, users had no way to check what request data is sent to the server before submitting a form. By calling get_request_kwargs(), the user can see the request data without having to send to.

hemberger · 2020-11-11T08:39:40Z

Thanks again for your contribution!

kumarstack55 · 2020-11-11T23:44:01Z

Thank you for merging. 😄

By the way, it occurs to me that this was the first time in my life that I've ever had my code reviewed.
I don't have a job writing programs. Furthermore, I had never even written a docstring before.
(I do some scraping as a hobby, so having Mechanicalsoup available would be very helpful. )

For me, all of the points you @hemberger @moy have given me are excellent. ✨
I'm sure it would be very difficult for you to review my code.
Thanks to your thoughtful comments, I managed to get it merged.

Again, thank you for the series of responses and for merging my request. 👍

moy · 2020-11-12T11:47:13Z

Thanks for the feedback. In another life, I'm a teacher, and that's what I try to explain my students: code review is not "I went through it and it doesn't look too bad", but can be much more picky and constructive than that ;-).

Previously, users had no way to check what request data is sent to the server before submitting a form. By calling get_request_kwargs(), the user can see the request data without having to send to.

hemberger requested changes Oct 15, 2020

View reviewed changes

mechanicalsoup/browser.py Outdated Show resolved Hide resolved

mechanicalsoup/browser.py Outdated Show resolved Hide resolved

kumarstack55 added a commit to kumarstack55/MechanicalSoup that referenced this pull request Oct 17, 2020

Add tests MechanicalSoup#339

49ec306

kumarstack55 added a commit to kumarstack55/MechanicalSoup that referenced this pull request Oct 17, 2020

Add method to raise TypeError when method or url in kwargs Mechanical…

536ce4a

…Soup#339

moy changed the title ~~Add _get_reuqest_kwargs to check before requesting~~ Add _get_request_kwargs to check before requesting Oct 17, 2020

moy requested changes Oct 17, 2020

View reviewed changes

kumarstack55 changed the title ~~Add _get_request_kwargs to check before requesting~~ Add get_request_kwargs to check before requesting Oct 18, 2020

kumarstack55 requested review from moy and hemberger October 18, 2020 10:34

kumarstack55 force-pushed the master branch from dcd6a38 to 4fde211 Compare October 19, 2020 13:12

moy approved these changes Oct 19, 2020

View reviewed changes

kumarstack55 force-pushed the master branch from 4fde211 to 7387736 Compare October 20, 2020 13:49

hemberger requested changes Nov 2, 2020

View reviewed changes

mechanicalsoup/browser.py Show resolved Hide resolved

Add get_request_kwargs() to check before submitting

f20513e

Previously, users had no way to check what request data is sent to the server before submitting a form. By calling get_request_kwargs(), the user can see the request data without having to send to.

kumarstack55 force-pushed the master branch from 7387736 to f20513e Compare November 8, 2020 05:54

hemberger merged commit e6da833 into MechanicalSoup:master Nov 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add get_request_kwargs to check before requesting #339

Add get_request_kwargs to check before requesting #339

kumarstack55 commented Oct 14, 2020

hemberger left a comment

kumarstack55 commented Oct 17, 2020

kumarstack55 commented Oct 17, 2020

moy commented Oct 17, 2020

moy left a comment

moy Oct 17, 2020

kumarstack55 Oct 18, 2020

moy Oct 17, 2020

kumarstack55 Oct 18, 2020

moy Oct 17, 2020

kumarstack55 Oct 18, 2020

moy Oct 17, 2020

kumarstack55 Oct 18, 2020

kumarstack55 commented Oct 18, 2020

kumarstack55 commented Oct 19, 2020

moy commented Oct 19, 2020

kumarstack55 commented Oct 19, 2020

kumarstack55 commented Oct 31, 2020

hemberger left a comment

hemberger commented Nov 11, 2020

kumarstack55 commented Nov 11, 2020

moy commented Nov 12, 2020

Add get_request_kwargs to check before requesting #339

Add get_request_kwargs to check before requesting #339

Conversation

kumarstack55 commented Oct 14, 2020

hemberger left a comment

Choose a reason for hiding this comment

kumarstack55 commented Oct 17, 2020

kumarstack55 commented Oct 17, 2020

moy commented Oct 17, 2020

moy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumarstack55 commented Oct 18, 2020

kumarstack55 commented Oct 19, 2020

moy commented Oct 19, 2020

kumarstack55 commented Oct 19, 2020

kumarstack55 commented Oct 31, 2020

hemberger left a comment

Choose a reason for hiding this comment

hemberger commented Nov 11, 2020

kumarstack55 commented Nov 11, 2020

moy commented Nov 12, 2020