pallets-eco
Fix misnamed class attribute
While scanning the code I noticed that there's a mismatched class attribute. On the base class it's user_enable, but on the subclasses and when checking in the template, it's user_activate. This patch is primarily focused on just fixing that simple typo - and adding tests that actually validate the attribute is doing what it claims to do (at least in so far as adding some HTML; it doesn't test functionality).
The @classmethod change may not be desirable and I'd gladly take a suggestion on how to get the dom ID without needing an instantiated class. May just be fine to build the ID again in the test, but that's obviously a little more brittle. Possibly also one you may consider a breaking change?
And I appreciate that it's a chunk of requirements changes for adding one more test, but they make querying the HTML for the elements under test a lot simpler, so hopefully it's not an issue.
Ah - I think this is possibly just a stale PR now.
I wrote this around the same time that I put up the host_matching change, which also used beautifulsoup4 for some tests. Those were since removed, and I didn't come back to update this.
I can update this and remove bs4 here too.
edit: although looking at refactoring the test, an equivalent test without bs4 does become a lot more painful and probably will end being less readable.
Thanks! If the other maintainers don't have concerns about the new dependencies, especially if we're planning to use them more and more for our testing suite, then I won't stand in the way.
I'm torn myself @macnewbold ... pulling in beautiful soup definitely does not make sense for just this single PR... but I can also see the benefit of having it available as a general testing utility available across the test suite. Ie, we would benefit from some scraping library, whether that beautiful soup or some other lib.
Also, it looks like this is only pulled in for tests here, so it's not actually pulled in for our downstream consumers. Given that, I feel much better about it... I'd be more hesitant if we start requiring it for downstream folks.
So overall I'm 👍 on this... I guess I view it as a directional bet that adding this as a test helper will reduce friction for writing tests => more/better tests down the road.
