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

Do not override encoding set during initialisation #1942

Merged
merged 1 commit into from
Aug 29, 2022
Merged

Do not override encoding set during initialisation #1942

merged 1 commit into from
Aug 29, 2022

Conversation

kp666
Copy link
Contributor

@kp666 kp666 commented Nov 15, 2019

encoding set during initialisation of sax parser gets replaced to ASCII when parsing.
Fix: check if encoding has already been set.

@flavorjones
Copy link
Member

Hi, there's a CI failure for this PR that I think is related, as it's an encoding test:

https://ci.nokogiri.org/teams/nokogiri-core/pipelines/nokogiri-pr/jobs/ruby-2.6-system/builds/60

I'd also like this PR to contain a test.

@kp666
Copy link
Contributor Author

kp666 commented Nov 26, 2019

@flavorjones I am not sure if to mutate encoding while passing encoding via parse_io.
please give directions here.

@codeclimate
Copy link

codeclimate bot commented Nov 26, 2019

Code Climate has analyzed commit 50d7495 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 50.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 46.3%.

View more on Code Climate.

@flavorjones
Copy link
Member

The current CI failures are not due to this commit -- will take a look now.

@flavorjones
Copy link
Member

ok - remaining failures would be addressed by a rebase onto master, regardless this is OK to merge ...

BUT I want to think about edge cases before doing so.

@kp666
Copy link
Contributor Author

kp666 commented Nov 27, 2019

My issue was that even when i set encoding to 'UTF-8' when the parser is initialised, parser.parse was calling parse_io without encoding. This meant that encoding was getting changed to 'ASCII' as per the old code.
IMO parse_io api seems a bit weird since parse_file and parse_memory apis do not take an additional encoding argument, and neither does mutate the 'encoding' instance variable, where as parse_io is capable of mutating encoding.

@flavorjones flavorjones added this to the v1.11.0 milestone Jan 11, 2020
@flavorjones
Copy link
Member

Just rebased onto master to get the checks green ...

@flavorjones
Copy link
Member

Rebased again to try to get CI green ...

Base automatically changed from master to main January 17, 2021 21:52
@flavorjones flavorjones modified the milestones: v1.12.0, v1.13.0 Aug 2, 2021
@flavorjones flavorjones modified the milestones: v1.13.0, v1.14.0 Jan 6, 2022
Previously, the encoding set during initialisation was set to ASCII
when parsing
@flavorjones
Copy link
Member

I've rebased this again onto current main

@flavorjones
Copy link
Member

LGTM! Merging. Thank you for your patience!!!

@flavorjones flavorjones merged commit 764748e into sparklemotion:main Aug 29, 2022
@flavorjones
Copy link
Member

I've credited you in the changelog in 243eab5. This will be in v1.14.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants