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

test extra space methods #132

Merged
merged 4 commits into from
Nov 20, 2024
Merged

test extra space methods #132

merged 4 commits into from
Nov 20, 2024

Conversation

mzelesky
Copy link
Member

@mzelesky mzelesky commented Nov 13, 2024

Tests extra_spaces? and extra_space_fix and moves the methods together in variable_fields.rb.
Completes testing of empty_subfields?

@mzelesky mzelesky marked this pull request as draft November 13, 2024 14:44
@mzelesky mzelesky changed the title [WIP] test extra space methods test extra space methods Nov 13, 2024
@mzelesky mzelesky marked this pull request as ready for review November 13, 2024 15:50
@mzelesky
Copy link
Member Author

@sandbergja Could you please review if you have time? I am inching closer to 100% coverage. These particular methods are deep in the weeds, but they do a lot to clean up presentation of records.

@mzelesky mzelesky requested a review from sandbergja November 13, 2024 15:55
Copy link
Member

@sandbergja sandbergja left a comment

Choose a reason for hiding this comment

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

Thanks, @mzelesky ! 🪈 I had one potential refactoring idea, definitely not a blocker.

Comment on lines 236 to 267
field_index = record.fields.index(field)
curr_subfield = -1
case field.tag
when /^[1-469]..|0[2-9].|01[1-9]|7[0-5].|5[0-24-9].|53[0-24-9]/
field.subfields.each do |subfield|
curr_subfield += 1
next if subfield.value.nil?

record.fields[field_index].subfields[curr_subfield].value = extra_space_gsub(subfield.value.dup)
end
when '533'
field.subfields.each do |subfield|
curr_subfield += 1
next if subfield.code == '7' || subfield.value.nil?

record.fields[field_index].subfields[curr_subfield].value = extra_space_gsub(subfield.value.dup)
end
when /^7[6-8]./
field.subfields.each do |subfield|
curr_subfield += 1
next if subfield.code =~ /[^a-v3-8]/ || subfield.value.nil?

record.fields[field_index].subfields[curr_subfield].value = extra_space_gsub(subfield.value.dup)
end
when /^8../
field.subfields.each do |subfield|
curr_subfield += 1
next if %w[w 7].include?(subfield.code) || subfield.value.nil?

record.fields[field_index].subfields[curr_subfield].value = extra_space_gsub(subfield.value.dup)
end
end
Copy link
Member

Choose a reason for hiding this comment

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

One potential refactor, which would remove the need to keep track of the curr_subfield and the field_index:

Suggested change
field_index = record.fields.index(field)
curr_subfield = -1
case field.tag
when /^[1-469]..|0[2-9].|01[1-9]|7[0-5].|5[0-24-9].|53[0-24-9]/
field.subfields.each do |subfield|
curr_subfield += 1
next if subfield.value.nil?
record.fields[field_index].subfields[curr_subfield].value = extra_space_gsub(subfield.value.dup)
end
when '533'
field.subfields.each do |subfield|
curr_subfield += 1
next if subfield.code == '7' || subfield.value.nil?
record.fields[field_index].subfields[curr_subfield].value = extra_space_gsub(subfield.value.dup)
end
when /^7[6-8]./
field.subfields.each do |subfield|
curr_subfield += 1
next if subfield.code =~ /[^a-v3-8]/ || subfield.value.nil?
record.fields[field_index].subfields[curr_subfield].value = extra_space_gsub(subfield.value.dup)
end
when /^8../
field.subfields.each do |subfield|
curr_subfield += 1
next if %w[w 7].include?(subfield.code) || subfield.value.nil?
record.fields[field_index].subfields[curr_subfield].value = extra_space_gsub(subfield.value.dup)
end
end
case field.tag
when /^[1-469]..|0[2-9].|01[1-9]|7[0-5].|5[0-24-9].|53[0-24-9]/
field.subfields.map! do |subfield|
next subfield if subfield.value.nil?
subfield.value = extra_space_gsub(subfield.value.dup)
subfield
end
when '533'
field.subfields.map! do |subfield|
next subfield if subfield.code == '7' || subfield.value.nil?
subfield.value = extra_space_gsub(subfield.value.dup)
subfield
end
when /^7[6-8]./
field.subfields.map! do |subfield|
next subfield if subfield.code =~ /[^a-v3-8]/ || subfield.value.nil?
subfield.value = extra_space_gsub(subfield.value.dup)
subfield
end
when /^8../
field.subfields.map! do |subfield|
next subfield if %w[w 7].include?(subfield.code) || subfield.value.nil?
subfield.value = extra_space_gsub(subfield.value.dup)
subfield
end

@mzelesky mzelesky requested a review from sandbergja November 18, 2024 16:44
@mzelesky
Copy link
Member Author

@sandbergja Can you take another look? It turns out I didn't need to keep track of the field and subfield indexes at all; I just modified it as part of the block.

Copy link
Member

@sandbergja sandbergja left a comment

Choose a reason for hiding this comment

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

Looks great to me, I like the refactor. Thanks, @mzelesky ! 🪈

@sandbergja sandbergja merged commit 3207790 into main Nov 20, 2024
2 checks passed
@sandbergja sandbergja deleted the extra_spaces branch November 20, 2024 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants