MechanicalSoup icon indicating copy to clipboard operation
MechanicalSoup copied to clipboard

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

Open johnhawkinson opened this issue 3 years ago • 2 comments

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.

johnhawkinson avatar Apr 18 '21 01:04 johnhawkinson

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!

johnhawkinson avatar Apr 18 '21 02:04 johnhawkinson

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. :)

hemberger avatar May 01 '21 18:05 hemberger