Skip to content
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

stateful_browser: self.url -> self.__state.url for internal consumers #367

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

johnhawkinson
Copy link
Contributor

Be consistent and use self.__state.url over the self.url() @Property for internal consumers.

Discussed somewhat in #362, the logic being that:

self.url is an interface for external callers to get access to the
internal state, and there's no reason to force the internal users to
do so.

There's extra cognitive load for readers of the code to follow
through the indirection.

Be consistent and use self.__state.url over the self.url() @Property for internal consumers.

Discussed somewhat in MechanicalSoup#362, the logic being that:

  self.url is an interface for external callers to get access to the
  internal state, and there's no reason to force the internal users to
  do so.

  There's extra cognitive load for readers of the code to follow
  through the indirection.
Copy link
Contributor

@hemberger hemberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I really appreciate your dedication to helping improve this library above and beyond what you need to get it to work for you.

Looking through the code more, I see that we have a similar inconsistency with self.page and self.form. One thing to note, however, is that the self.form property does more than simply return self.__state.form: it also checks to make sure a form has been selected. Therefore, we will likely need to retain most references to self.form, which prevents us from using the __state attributes consistently, even if we were to convert self.page.

Does that change your evaluation of this situation at all? Maybe it hints at a deeper design flaw.

@moy Do you have any opinions on this matter?

@johnhawkinson
Copy link
Contributor Author

Looking through the code more, I see that we have a similar inconsistency with self.page and self.form. One thing to note, however, is that the self.form property does more than simply return self.__state.form: it also checks to make sure a form has been selected. Therefore, we will likely need to retain most references to self.form, which prevents us from using the __state attributes consistently, even if we were to convert self.page.

That's a good question! Are those checks appropriate in these cases?
For instance, select_form() sets self.__state.form before evaluating self.form, so it's redundant.
It looks like all the other ones are necessary (or, at least, meaningful and probably wise): __setitem__(), new_control(), and submit_selected().

I guess this means I'm wrong and we should go with self.url in all cases?

Or maybe we should leave well-enough-alone, I dunno!

@hemberger
Copy link
Contributor

I agree, there's definitely not an obvious answer here. As you noted, in some places it is important to use self.form, but in others it's redundant (or even incorrect, e.g. when we're setting self.__state.form manually in select_form). So it's clear we're going to have a mix no matter what we do. Perhaps the most consistent way to do it would be to distinguish between "using" and "setting", which I think would mean using the properties everywhere except in select_form (and obviously the @property methods as well).

You are certainly not responsible for making this all consistent, so if this is just a headache you want to forget, that's totally okay. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants