Override submit method in StatefulBrowser

[hemberger]

stateful_browser.py: rename submit_selected to submit …

The obvious method to use when trying to submit a form is the submit
method. Unless you are following the documentation explicitly, you
may overlook the submit_selected method (which is really the only
correct method to use when submitting a form within a StatefulBrowser
instance).

This is demonstrated by #230, where the user got an unexpected error
because they used submit instead of submit_selected.

To adhere to the principle of least astonishment, make submit a
synonym for submit_selected (with submit being the primary name
and submit_selected being an alias for backwards compatibility).

This is a breaking change for anyone calling the base class submit
from a StatefulBrowser instance. It is arguable that even if the call
is made correctly, it is the "wrong" way to submit, so we should make
it more difficult to "do the wrong thing".

You can still access the base class Browser.submit from an instance
of StatefulBrowser if you need to by using the super builtin, e.g.

br = mechanicalsoup.StatefulBrowser()
super(StatefulBrowser, br).submit(...)

NOTE: I've also added a commit to improve backwards compatibility and
issue a deprecation warning if a user tries to call Browser.submit
from a StatefulBrowser instance if they are passing a Form instance
(we can't catch cases where they are passing a bs4.element.Tag -- see
the commit message for details).

[hemberger]

This pull request introduces 1 alert when merging f681e7b into 730ca4a - view on LGTM.com

new alerts:

• 1 for Mismatch between signature and use of an overridden method

---

Comment posted by LGTM.com

[moy]

Unfortunately, forbidding this is not only a backward-compatibility problem, but also a violation of Liskov's Substitution Principle. The base .submit() accepts a btnName instance of Form, so all derived methods should also accept at least that (and possibly more), so that anything doable with a Browser is also doable with a StatefulBrowser.

Also, having the same positional argument be btnName in one class and form in another is very misleading. LGTM is right to complain here IMHO.

[hemberger]

Before I invest too much more time into this, I just wanted to check if you are generally opposed to this PR, or if you can imagine a modified PR that you would consider accepting. Even better if you already have an idea in mind! ;)

My preference is always to make StatefulBrowser the best class it can be. The Browser class sometimes gets in the way of that, such as in this case. I believe StatefulBrowser would benefit from a submit method, so if we can't override Browser.submit in the proposed way, the first two alternatives that come to mind (setting aside the issue of backwards compatibility for a moment) are renaming Browser.submit to either a) Browser._submit or b) Browser.submit_form.

[moy]

If I were to redo this from scratch, I'd consider using composition instead of inheritance, i.e. consider that a StatefulBrowser contains a Browser, not that it is a Browser. It'd be hard to change that without breaking backward compatibility, though.

Renaming Browser.submit is also problematic with respect to backward compatibility. Perhaps not many people call submit on a StatefulBrowser, but everyone who wrote code before StatefulBrowser was added does call submit on a Browser.

There's another option: add warnings for suspicious cases, and allow the user to disable the warnings as needed. We could warn on calls to StatefulBrowser.submit, or if we want to go further, on creation of a Browser that isn't a StatefulBrowser. Probably not as a deprecation warning like "what you're doing won't work anymore soon", but just like "uh, are you sure you want that?".

[hemberger]

A warning for suspicious use is a great idea, but I'm not sure that it addresses the main issues here (and might be just as much of an annoyance as a modified interface to some users). Out of curiosity, though, did you have a design in mind for warning toggling?

Okay, two more possibilities:

1. Simply add a comment to the Browser.submit docstring saying something like:

If you are calling this method from a StatefulBrowser instance, consider using StatefulBrowser.submit_selected instead.

2. Override Browser.submit in a Liskov-compliant way, e.g.

def submit(self, form=None, url=None, **kwargs, btnName=None):

where form and url default to the current form/url if None. It's a bit of a clunky interface, but might be the best we can do if we want to make overriding work within our constraints. Or even just

def submit(self, form=None, url=None, **kwargs):

to avoid complications with btnName.

[hemberger]

@moy I'd like to get version 0.11 released as soon as possible, but I want to address this issue in some way before I do, even if it's just a minor addition to the documentation.

How about I make the documentation change proposed above for 0.11, and then we can reassess if we want to make any code changes for version 1.0 later?

[moy]

The doc change is obviously a good step forward.

Your option 2. is probably doable too. I won't have time to look at this in details soon, but if you're confident enough I trust you to do the right thing.

[hemberger]

Thanks for the feedback! For the 0.11 release I'll just change the docstring, unless you recommend otherwise.

As I think more about it, overriding submit seems increasingly important. One of the biggest issues (that I didn't appreciate until just now) is that if you call submit on a StatefulBrowser, the browser state becomes stale. I would argue that this is a pretty dangerous inconsistency for a method that looks like the most obvious way to submit a form.

[dillonko]

@moy and @hemberger With regards to how old this PR is and whether it's a valid issue any more should this be closed?