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

Fix misnamed class attribute #268

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

Conversation

samuelhwilliams
Copy link
Contributor

@samuelhwilliams samuelhwilliams commented Jun 9, 2024

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.

Copy link
Contributor

@macnewbold macnewbold left a comment

Choose a reason for hiding this comment

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

The meat of the change is a one-line typo correction, which looks good to me. There's also a switch for the dom_id() method to be a class method instead of instance method. I'm not sure the purpose or driver of that change, but it seems reasonable enough.

The rest of the PR is adding a unit test for the change (great!) and a bunch of new dependencies, primarily due to the addition of beautifulsoup4 and its dependencies. That's the only part of this PR that gives me any pause at all. What do others think about the additional dependencies?

@samuelhwilliams
Copy link
Contributor Author

samuelhwilliams commented Aug 3, 2024

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.

@macnewbold
Copy link
Contributor

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.

@jeffwidman
Copy link
Member

jeffwidman commented Aug 12, 2024

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.

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

Successfully merging this pull request may close these issues.

3 participants