-
Notifications
You must be signed in to change notification settings - Fork 376
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
Open
hemberger
wants to merge
2
commits into
MechanicalSoup:main
Choose a base branch
from
hemberger:rename-submit-selected
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back 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.
Unfortunately, forbidding this is not only a backward-compatibility problem, but also a violation of Liskov's Substitution Principle. The base
.submit()
accepts abtnName
instance ofForm
, so all derived methods should also accept at least that (and possibly more), so that anything doable with aBrowser
is also doable with aStatefulBrowser
.Also, having the same positional argument be
btnName
in one class andform
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. TheBrowser
class sometimes gets in the way of that, such as in this case. I believeStatefulBrowser
would benefit from asubmit
method, so if we can't overrideBrowser.submit
in the proposed way, the first two alternatives that come to mind (setting aside the issue of backwards compatibility for a moment) are renamingBrowser.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 aBrowser
, not that it is aBrowser
. 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 callsubmit
on aStatefulBrowser
, but everyone who wrote code beforeStatefulBrowser
was added does callsubmit
on aBrowser
.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 aBrowser
that isn't aStatefulBrowser
. 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:
Browser.submit
docstring saying something like:Browser.submit
in a Liskov-compliant way, e.g.where
form
andurl
default to the current form/url ifNone
. 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 justto 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 callsubmit
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.