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
Override submit method in StatefulBrowser #233
base: main
Are you sure you want to change the base?
Conversation
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 MechanicalSoup#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(...)
In the (likely) most common scenario of an intended call to the `Browser.submit` method from a StatefulBrowser instance, add a backwards-compatible passthrough to `Browser.submit` in the new `StatefulBrowser.submit` method while also issuing a deprecation warning. The way this works is that it checks to see if `btnName` is an instance of the `Form` class, in which case it is unambiguously intended to be a call to `Browser.submit`. This should fix the majority of cases where users may have been affected by the breaking change of overriding the `submit` method in StatefulBrowser. We cannot fix the remaining case, which is when `Browser.submit` was passed a bs4.element.Tag with name attribute "form". The reason we cannot create a similar passthrough for this is that there may be a legitimate `btnName` that is a bs4.element.Tag with name="form". This is unlikely (since submit elements typically would not be named "form"), but is technically possible.
This pull request introduces 1 alert when merging f681e7b into 730ca4a - view on LGTM.com new alerts:
Comment posted by LGTM.com |
warnings.warn("This usage of StatefulBrowser.submit is deprecated." | ||
" Please see the documentation for this function to " | ||
"upgrade to the new interface.", | ||
DeprecationWarning) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
- Simply add a comment to the
Browser.submit
docstring saying something like:
If you are calling this method from a
StatefulBrowser
instance, consider usingStatefulBrowser.submit_selected
instead.
- 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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
This docstring note is a necessary (but probably not sufficient) change to `Browser.submit` to prevent it from being used in an inappropriate way from a `StatefulBrowser` instance. See MechanicalSoup#230 and MechanicalSoup#233 for more info.
This docstring note is a necessary (but probably not sufficient) change to `Browser.submit` to prevent it from being used in an inappropriate way from a `StatefulBrowser` instance. See MechanicalSoup#230 and MechanicalSoup#233 for more info.
@moy and @hemberger With regards to how old this PR is and whether it's a valid issue any more should this be closed? |
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 onlycorrect 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 ofsubmit_selected
.To adhere to the principle of least astonishment, make
submit
asynonym for
submit_selected
(withsubmit
being the primary nameand
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 instanceof
StatefulBrowser
if you need to by using thesuper
builtin, e.g.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).