SongProOrg / songpro-ruby

A Ruby Gem to convert SongPro songs to a Ruby object used to generate various output formats.
https://songpro.org
MIT License
5 stars 1 forks source link

Trimming whitespaces around lyrics intentional? #2

Closed Dahie closed 4 years ago

Dahie commented 4 years ago

Hi,

I don't know if the following is an issue or a conscious decision. Let's assume the following rspec test:

context 'chords and lyrics' do
  it 'parses lyrics before chords' do
    song = SongPro.parse("It's a[D]bout a [E]boy")
    expect(song.sections.size).to eq 1
    expect(song.sections[0].lines.size).to eq 1
    expect(song.sections[0].lines[0].parts.size).to eq 3
    expect(song.sections[0].lines[0].parts[0].chord).to eq ''
    expect(song.sections[0].lines[0].parts[0].lyric).to eq "It's a"
    expect(song.sections[0].lines[0].parts[1].chord).to eq 'D'
    expect(song.sections[0].lines[0].parts[1].lyric).to eq 'bout a  '
    expect(song.sections[0].lines[0].parts[2].chord).to eq 'E'
    expect(song.sections[0].lines[0].parts[2].lyric).to eq 'boy'
  end
end

This will fail, because the expectation expect(song.sections[0].lines[0].parts[1].lyric).to eq 'bout a ' actually returns 'bout a' with stripped whitespaces at the end. Is this intentional?

In the software I'm writing at the moment, it does make a difference if a chord is within a word or before a word. With this behaviour, the example is compacted to: 'It's about aboy'

Dahie commented 4 years ago

Hi @spilth. maybe this also slipped through the notifications. What are your thoughts here? :)

spilth commented 4 years ago

Hey @Dahie. The stripping is intentional but I don't remember my reasoning behind it. I suspect it has something to do with how I'm displaying things in HTML, but I'm not entirely certain.

If you remove the stripping behavior, does the test suite still pass?

Dahie commented 4 years ago

If I make the test above pass, there are 3-4 additional scenarios that will need adjustment in their expectations. For code, see https://github.com/SongProOrg/songpro-ruby/pull/3

spilth commented 4 years ago

@Dahie I'm gonna need to spend some time making this change locally and seeing if affects my other project that relies on this Gem - https://github.com/spilth/mss.nyc

spilth commented 4 years ago

Hey @Dahie. I pulled in your branch, installed the gem locally and tried my site with it and all seems to be well. So I'm gonna merge this in. If there ends up being a problem we can always revert the change.

I'm also gonna update the Ruby version, update any dependencies that need it and push out a new version.

spilth commented 4 years ago

Fixed by @Dahie in #3 and released as 0.1.7: https://rubygems.org/gems/song_pro/versions/0.1.7