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

[bug] Sanitize pin and wire labels so they are interpreted as strings #305

Open
EvilMustacheTwirl opened this issue Jan 27, 2023 · 5 comments

Comments

@EvilMustacheTwirl
Copy link

While using WireViz to document cables to several SPDT lever switches and relays. I labeled the pins on the switch as the standard labelspinlabels = [NO,NC,COM] and then spent an hour trying to figure out why is was getting the string expected got bool error. The label NO was being interpreted as a bool and not a string.

Workaround was to quote 'NO' and then it correctly interpreted as a string.

Since the errors don't report where in the input file the issue is and unless there's a reason that labels should be interpreted as a bool, int, etc. I think it would be a good idea to just convert the list items in the label classes as strings.

@kvid
Copy link
Collaborator

kvid commented Jan 27, 2023

Thank you for reporting this YAML side effect. It seems to be how the current YAML parser is intended to work. It might change in the future, e.g. if deciding to use a parser supporting YAML version 1.2 - where the unquoted words Yes/No and On/Off are no longer interpreted as aliases for the boolean values true/false. See also the related issues #213 and #223.

Your idea to convert the label items to string is not that easy, because the YAML parser library takes the total YAML input as one large string input, parses it according to the YAML 1.1 language specification, and returns the total result as Python values (dict, list, string, bool, int, etc.) To override how to interpret the type of these unquoted words, will have these effects:

  • It's a global change and thus applies to all YAML input.
  • The WireViz input language no longer conforms to the YAML 1.1 specification.

@kvid
Copy link
Collaborator

kvid commented Feb 3, 2024

@EvilMustacheTwirl wrote:

Since the errors don't report where in the input file the issue is [and ...]

I agree that reporting "where in the input file the issue is" would help identifying the root cause. The same also applies for other error messages. The parser we use doesn't report line numbers, but as suggested in #207, we could report more context information in our error messages. Issue #207 has been used as a common thread for such kind of issues.

@martinrieder
Copy link
Contributor

martinrieder commented May 17, 2024

It seems to be how the current YAML parser is intended to work. It might change in the future, e.g. if deciding to use a parser supporting YAML version 1.2 - where the unquoted words Yes/No and On/Off are no longer interpreted as aliases for the boolean values true/false. See also the related issues #213 and #223.

To override how to interpret the type of these unquoted words, will have these effects:

  • It's a global change and thus applies to all YAML input.
  • The WireViz input language no longer conforms to the YAML 1.1 specification.

While the discussion on error output is held in #207, we should keep this one open for the suggested YAML parser replacement.

@EvilMustacheTwirl is making a good point. Interpreting everything as a string could help fix some issues. Numbers and Booleans should be evaluated only where the WireViz syntax requires this. Would a simple regex replacement hack possibly also do the job?

@martinrieder
Copy link
Contributor

martinrieder commented May 17, 2024

@kvid I noticed the comment on YAML parser improvements recommends that I should rather take my previous suggestion over to #186. However, it has already been merged. How shall handle any further improvements on YAML parsing?

Edit: The YAML parsing is being completely refactored in #186, so any attempts to improve the code should start from that branch, or wait until that PR is merged into dev.

@formatc1702 in #207 (comment)

All comments have been addressed inside #251.

Not merging yet since it would immediately push dev forward, but without the latest code suggestions. I will wait until #251 is merged to have the suggestions make it directly into dev as well.

@formatc1702 in #186 (comment)

@kvid
Copy link
Collaborator

kvid commented May 17, 2024

#186 was merged to dev last year, and is now included in v0.4. #251 is next in line for developing against v0.5 with some heavy refactoring of a large part of the code.

#223 was created to discuss a switch of YAML parser, but without much responce yet.

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

No branches or pull requests

3 participants