Skip to content

config.types: add SetAttribute type, based on ListAttribute#2689

Draft
dgw wants to merge 2 commits intomasterfrom
SetAttribute
Draft

config.types: add SetAttribute type, based on ListAttribute#2689
dgw wants to merge 2 commits intomasterfrom
SetAttribute

Conversation

@dgw
Copy link
Copy Markdown
Member

@dgw dgw commented Jul 22, 2025

Description

Implements the new config type idea from #2687 (comment)

Most of the parsing/serializing code is just inherited, with a thin layer of tweaks on top to return the correct type.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make lint and make test)
    • I didn't have a chance to lint, sadly. (I hate travel days, but I don't mind leaning on CI sometimes.)
  • I have tested the functionality of the things this change touches
    • via tests, naturally

Notes

Draft until #2687 gets merged, because that's the first usage site for this and there's not much point in adding the feature without something in core that uses it.

Most of the parsing/serializing code is just inherited, with a thin
layer of tweaks on top to return the correct type.
@dgw dgw added this to the 8.1.0 milestone Jul 22, 2025
@dgw dgw added the Feature label Jul 22, 2025
@dgw
Copy link
Copy Markdown
Member Author

dgw commented Aug 3, 2025

After converting the .blocks lists to SetAttribute (needs #2687 merged first to avoid unnecessary merge conflicts), we should audit all the .blocks code again for usage of strip() or other likely-unnecessary processing steps that the SetAttribute itself should handle. (The existing ListAttribute code probably already makes those unnecessary, but I'm deferring the audit in the interest of—again—reducing PR churn because of conflicts.)

Edit: Also, use of str() casts as mentioned here:

Nit: Huh, do we expect not-str here for some reason? Wondering why this isn't:

But I guess this question extends to the other branches too since it's clearly written to match the existing code. Perhaps handling Identifier got a bit infectious here, or it's done in the name of strict symmetry?

@dgw
Copy link
Copy Markdown
Member Author

dgw commented Aug 3, 2025

More things to audit after converting to SetAttribute:

Copy link
Copy Markdown
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

Sure! 😁

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants