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

Fixing decode error of + strings (plus sign) #14

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

TBeijen
Copy link

@TBeijen TBeijen commented Feb 3, 2024

Fix for #13

The first regex OR (now removed) caused regex to pass even if none of the succeeding oct|dec|hex definitions was present.

Quite confident this fixes it:

  • Testsuite contains tests for oct|dec|hex (decode_test.go)
  • No unexpected side-effects in testsuite

@TBeijen TBeijen changed the title WIP: fixing decode error of + strings (plus sign) Fixing decode error of + strings (plus sign) Feb 3, 2024
@TBeijen TBeijen marked this pull request as ready for review February 3, 2024 14:14
@apparentlymart
Copy link
Contributor

apparentlymart commented May 18, 2024

Hi @TBeijen! Thanks for working on this and sorry for the very slow response.

I agree that this seems like a more accurate read of the rules in Tag Resolution: it requires there to be at least one digit after the + or - to count as an tag:yaml.org,2002:int, but this was previously allowing zero digits.

Looking closely at that specification text, I think there's also another inconsistency: the [-+]? part is included only for the decimal form, and not for the octal and hex forms. Therefore I think the [-+]? pattern ought to be inside the disjunction directly before the [0-9_]+ pattern.

In other words, I think the correct pattern is (0o[0-7_]+|[-+]?[0-9_]+|0x[0-9a-fA-F_]+). Would you agree?

(The spec doesn't mention allowing underscores in any of these, so I'm not really sure where that came from in #6 now that I look at it in retrospect. Perhaps we should drop those too, because otherwise _ would be interpreted as an integer and that doesn't seem right. 🤔 )

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