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

Assume undefined icons to be None (GTK) #2024

Closed
wants to merge 0 commits into from
Closed

Assume undefined icons to be None (GTK) #2024

wants to merge 0 commits into from

Conversation

volxage
Copy link

@volxage volxage commented Jul 4, 2023

Instead of just checking if the row has an icon, the new proposed conditional checks if the "icon" attribute is a member of the row at all, then checks if the "icon" attribute is None. The code is also slightly refactored as if not hasattr... is not as readable (to me) as getattr(...) is not None

Previously, without setting a row's icon either explicitly, an error was thrown because getattr(row, "icon") is looking for an attribute that wasn't provided. After this change, when rows are defined without the icon attribute, the rows render without an icon and the code doesn't throw an error. This is only for GTK's implementation. (My first ever pull request, I wanted to start small :)

PR Checklist:

  • All new features have been tested
    (NA) All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

Copy link
Member

@freakboy3742 freakboy3742 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!

CI is currently failing because the PR is missing a change note.

In terms of the code itself - our general code style is to avoid hasattr() in favor of try/except AttributeError blocks.

I also want to flag that I've just started an audit of the DetailedList widget - see #2011 and #2017 for analogous audits of Table and Tree. These tests include checks of "has icon", "has no icon", and "has empty icon", which will should catch issues like this one.

@volxage volxage closed this Jul 5, 2023
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