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.

Sign up for GitHub

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

Jump to bottom

Parser::SourceParser#convert_encoding - fixup BOM encoding #1510

Merged
merged 1 commit into from Sep 30, 2023
Merged

Parser::SourceParser#convert_encoding - fixup BOM encoding #1510

merged 1 commit into from Sep 30, 2023

Conversation

MSP-Greg
Copy link
Contributor

Description

Ruby 3.3 may change handling of BOM encoded files. Change code to work with 3.3 and earlier versions.

Closes #1509

Note that Ruby 2.7 added IO##set_encoding_by_bom

Completed Tasks

  • I have read the Contributing Guide.
  • The pull request is complete (implemented / written).
  • Git commits have been cleaned up (squash WIP / revert commits).
  • I wrote tests and ran bundle exec rake locally (if code is attached to PR).

@MSP-Greg MSP-Greg mentioned this pull request Sep 24, 2023
Closed
lsegal
lsegal requested changes Sep 30, 2023
View reviewed changes
Copy link
Owner

@lsegal lsegal left a comment

Choose a reason for hiding this comment

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

I think this would be good if it retained the initial equality check.

lib/yard/parser/source_parser.rb
if content[0, bom.size] == bom
content.force_encoding(encoding)
return content
if content.start_with?(bom)
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure there's a good enough reason to switch to this more restrictive (newer) API given the simplicity of the existing line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it for two reasons, first, content[0, bom.size] creates a string, second, both content and bom are both forced to binary encoding, so there won't be any encoding issues with a character based test, which also happens with the existing check. Not sure why you're calling it 'more restrictive'?

Regardless, I can revert that change if you'd like...

Copy link
Owner

Choose a reason for hiding this comment

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

I remember start_with? being added in Ruby 2.5+, but it seems like it was added much earlier-- in which case this is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember start_with? being added in Ruby 2.5+

My memory isn't good for that sort of thing. CI did pass... I remember a starts_with? a very long time ago. Thanks.

@lsegal lsegal merged commit abcba4b into lsegal:main Sep 30, 2023
25 checks passed
@lsegal
Copy link
Owner

lsegal commented Sep 30, 2023

Thanks for the quick fix on #1509!

@MSP-Greg MSP-Greg deleted the 00-ruby3.3-bom branch September 30, 2023 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lsegal lsegal lsegal approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ruby3.3 causes failure for YARD::Parser::SourceParser#parse understands UTF-8 BOM
2 participants
@MSP-Greg @lsegal