Conversation
HassanAkbar
force-pushed
the
source-details-parsing
branch
from
6d6efff to
a2532a0
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces structured parsing for the <source details="..."> attribute by adding a Termium::SourceDetails value object and wiring it into Termium::Source.
Changes:
- Add
Termium::SourceDetailswith casting/serialization and asterisk-delimited parsing. - Update
Termium::Source#detailsto useSourceDetailsand adjust#contentformatting accordingly. - Add RSpec coverage for parsing, formatting, and XML round-tripping.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/termium/source_spec.rb | Adds tests for Termium::Source XML round-trip and #content formatting. |
| spec/termium/source_details_spec.rb | Adds unit tests for Termium::SourceDetails casting/serialization behavior. |
| lib/termium/source_details.rb | Introduces SourceDetails parser/type for the details attribute. |
| lib/termium/source.rb | Switches details attribute to SourceDetails and updates matching logic. |
| lib/termium.rb | Adds require_relative for source_details. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/termium/source_details.rb
Outdated
| if @organization | ||
| @author_name = presence(columns[0]) | ||
| else | ||
| @standard = presence(columns[0]) | ||
| end |
There was a problem hiding this comment.
SourceDetails currently decides whether the first column is a standard vs author_name based on whether @organization (column 3) is present. Real fixture data includes entries like details="ISO/IEC JTC 1 * 2011" (see spec/fixtures/Characters.xml), where there is no 3rd column but the first column is not an ISO(-IEC)-* standard identifier. With the current logic this gets incorrectly stored in standard. Consider identifying standards by pattern (e.g., ISO-… / ISO-IEC-… with a document number), and otherwise treating the first column as a non-standard author/organization value.
| end | ||
|
|
||
| def serialize(value) | ||
| value&.raw |
There was a problem hiding this comment.
SourceDetails.serialize currently returns value&.raw, which will return nil if value is a plain String. That can drop data if callers set details to a raw string (or if the serializer passes a string through). Consider handling String inputs explicitly (e.g., returning the string as-is) in addition to SourceDetails instances.
| value&.raw | |
| return nil if value.nil? | |
| return value.raw if value.is_a?(SourceDetails) | |
| return value if value.is_a?(String) | |
| value |
There was a problem hiding this comment.
The value parameter is expected to be a SourceDetails instance or nil, never a plain String. Strings are converted to SourceDetails via cast before serialize is called.
spec/termium/source_details_spec.rb
Outdated
|
|
||
| RSpec.describe Termium::SourceDetails do | ||
| describe ".cast" do | ||
| context "with ISO standard (no organization)" do |
There was a problem hiding this comment.
The context description says "with ISO standard (no organization)", but the input value is an ISO-IEC identifier ("ISO-IEC-2382-16 ..."). Updating the description to match the data will make the spec intent clearer.
| context "with ISO standard (no organization)" do | |
| context "with ISO/IEC standard (no organization)" do |
1 participant
fixes #1