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

raise ArgumentError on unknown directive for Array pack / String unpack #3727

Merged

Conversation

Th3-M4jor
Copy link
Contributor

@Th3-M4jor Th3-M4jor commented Nov 24, 2024

Makes it so that Array#pack and String#unpack will raise an ArgumentError on unknown directives.

Since it appears that ruby/spec does not have any specs for unknown directives on String#unpack, I opened a PR there to add one.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Nov 24, 2024
@andrykonchin
Copy link
Member

andrykonchin commented Nov 25, 2024

Looks good to me.

I suppose the change fixes both #pack and #unpack methods. Could you please add changes of the ruby/spec#1218 PR into this one? ruby/spec's tests in TruffleRuby are being synced periodically with the main ruby/spec Git repo.

Also it makes sense to specify exception message (unknown pack directive ...) in the new test and probably in the existing one.

@Th3-M4jor Th3-M4jor force-pushed the array-pack-raise-unknown-directives branch from 8ca3bd5 to f64da80 Compare November 25, 2024 23:45
@Th3-M4jor
Copy link
Contributor Author

Had to make a slight change to the SimplePackListener interface so that the error messages would more closely match those from MRI as it differentiates between pack/unpack in the error message.

Copy link
Member

@andrykonchin andrykonchin left a comment

Choose a reason for hiding this comment

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

Thank you!

@andrykonchin andrykonchin added the in-ci The PR is being tested in CI. Do not push new commits. label Nov 26, 2024
@graalvmbot graalvmbot merged commit 817b7b9 into oracle:master Nov 27, 2024
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-ci The PR is being tested in CI. Do not push new commits. OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants