inifile: accept dash in section and variable identifiers - #4573
grandixximo wants to merge 1 commit into
Conversation
The new ini parser restricts identifiers to letters, digits and underscore, rejecting names containing a dash. The historic parser had no identifier character restrictions, so files using dashes parsed fine for decades and now fail: - the xhc-hb04 sample configs use dashed button names in [XHC_HB04_BUTTONS] (start-pause, goto-zero-x, ...), which makes these configs fail to load - the xhc-hb04 component's own layout cfg files use a [XHC-HB04] section, so the component exits at startup with "Could not open configuration file" A dash is unambiguous in this grammar (it is neither a delimiter, nor a comment or quoting character), so accept it in identifiers. Identifiers starting with a dash remain rejected, like those starting with a digit. Update the identifier documentation accordingly, extend the inivalue test with leading-dash rejection cases, and add a hyphen_identifiers test covering dashed section and variable names.
|
It was intentional to drop the '-' and restrict the names to a real identifier. The pendent was obviously missed. It also uses a very strange mismatch of underscore and dash. What is even more telling is that the example int It is IMO a bit messy. Not sure whether we should keep it by adding the dash or actually clean up and refactor the code and process. |
|
Thanks for the context. On the cleanup question, I looked at what a refactor would actually touch. The dashed names are the documented, user-facing format in two places:
Since the strict parser fails the whole file on the first dashed identifier, a rename to underscores makes all existing user INI and cfg files unreadable on upgrade. That said, 2.10 already breaks configs elsewhere (#4565, with #4499 and #4256 as migration tooling), so a deliberate rename of the pendant identifiers is not off the table. The button names live in the HAL pin names too (xhc-hb04.button-start-pause), so a rename would need migration coverage for INI keys, the layout cfg and HAL files. That feels like a decision for the Sunday meeting (agenda in #4566) rather than something to settle in this PR. Whichever way that goes, this PR restores the pre-refactor behavior for the files that exist today, and a rename with migration support can still land on top if the meeting decides for it. If the decision is to keep the dash, the only cosmetic cleanup left is:
Happy to do that as a follow-up, or to work on the full rename if the meeting prefers that route. Converting this PR to draft until there is a decision. |
2 participants
The new ini parser restricts identifiers to letters, digits and underscore, which rejects names containing a dash. The historic parser had no identifier character restrictions, so dashed names parsed fine for decades. Two things break on master because of this:
Renaming the identifiers is not a good option: the [XHC-HB04] section name is hardcoded in xhc-hb04.cc, and user pendant cfg files in the wild use the dashed layout format.
A dash is unambiguous in this grammar (not a delimiter, comment or quoting character), so this PR accepts it in identifiers. Identifiers starting with a dash remain rejected, same as those starting with a digit.
Docs updated (ini-config.adoc, inivalue.1.adoc), the inivalue test is extended with leading-dash rejection cases, and a new hyphen_identifiers test covers dashed section and variable names. Full inifile suite passes 11/11, and all four xhc-hb04 sample configs boot with the pendant's hal pins present.
@BsAtHome: was dropping the dash from the identifier set an intentional choice, or an omission? If it was deliberate (some ambiguity I am missing), the alternative is fixing the xhc-hb04 side instead, but that changes a long-documented cfg format and breaks existing user configs, so I would rather not.