Optimize input, add parsing options, add rake gem #1
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes XML parsing performance by implementing memory-efficient input handling for large XML strings and adding flexible text processing options.
- Introduces StringIO optimization for XML strings larger than 1MB to reduce memory allocation
- Adds configurable options for key symbolization and whitespace handling (
symbolize_keys,strip_whitespace,normalize_whitespace) - Bumps version from 0.5.1 to 0.6.0
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/oxml/version.rb | Version bump to 0.6.0 |
| lib/oxml/parser.rb | Adds new parsing options and implements conditional key symbolization with whitespace processing |
| lib/oxml.rb | Introduces StringIO optimization for large XML inputs with 1MB threshold |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
evgenlytkin
changed the title
xkwd
left a comment
xkwd
left a comment
There was a problem hiding this comment.
This is a nice addition! 👍🏻 I would appreciate if you could briefly mention the changes in the changelog and, if possible, in the readme, at least the fact that new options are being added
| def optimize_xml_input(xml) | ||
| return xml unless xml.is_a?(String) && xml.bytesize > IO_OPTIMIZATION_THRESHOLD | ||
|
|
||
| StringIO.new(xml) | ||
| end |
There was a problem hiding this comment.
Would you be able to add a test for when this threshold is met to make sure we have spec better spec coverage?
| @@ -0,0 +1 @@ | |||
| require "bundler/gem_tasks" | |||
There was a problem hiding this comment.
Is it a leftover? I don't see any tasks or anything related 🤔
| spec.files = Dir['lib/**/*'] | ||
| spec.required_ruby_version = '>= 2.6' | ||
| spec.add_dependency 'ox', '~> 2.14' | ||
| spec.add_development_dependency 'rake', '~> 13.2' |
| @symbolize_keys = options.fetch(:symbolize_keys, true) | ||
| @strip_whitespace = options.fetch(:strip_whitespace, false) | ||
| @normalize_whitespace = options.fetch(:normalize_whitespace, false) |
There was a problem hiding this comment.
Would you be able to add spec coverage for these three new options? We already have it for every option above
Context
Parsing XML responses can increase latency and allocate too much memory due to their size. For this reason, we need to add tweaks to mitigate its aftermath.
Introduced changes