raise an exception when align value is not a Symbol

[lucianosousa]

No description provided.

[pointlessone]

@lucianosousa Hi! Thank you for your interest in giving back to community!

Could you please give a bit of context for this PR?

[lucianosousa]

@pointlessone sure man.
I was trying to align here with align: 'center' and the pdf render just broke.
Some rails methods accept both kinds of argument types, eg: https://github.com/rails/rails/blob/master/activemodel/lib/active_model/validations.rb#L256
we can do the same here, don't you think?

[pointlessone]

Prawn is not a part of the Ruby on Rails framework. We do not quite share an outlook on API design with Rails.

I agree that it's suboptimal to break document rendering. But I think a proper solution here to let the user know what's wrong instead of extending the API. We generally do not accept strings where only a limited number of named options can be accepted and I don't want to diverge from that in this case.

What do you think of an alternative solution? Add an else to that case that will raise an ArgumentError with a descriptive message.

[lucianosousa]

@pointlessone cool. better raise an exception then ;)

[pointlessone]

@lucianosousa Would you like to change the code in this PR or l it to someone else?

[lucianosousa]

@pointlessone sorry the delay. it's here! ;)

[pointlessone]

@lucianosousa I would very much prefer if the exception was risen in every unrecognized case not only when it's not a symbol.

[lucianosousa]

sorry being late. updated

[pointlessone]

NP. Thank you for keeping working on this.

Could you please put the raise into else branch of the case statement to save us extra check?

[pointlessone]

• The unless part is not needed any more. It's covered by the preceding case branches.
• You also can write valid values in Ruby syntax (e.g. :left). I believe developers would pick that up.
• I'm not a native speaker myself but overall wording is a bit awkward to me. What do you think of the following: align must be one of :left, :right, :center or :justify

[lucianosousa]

done @pointlessone sorry being late

[pointlessone]

Well done!

[pointlessone]

Could you please fix code style to make RuboCop happy?

[lucianosousa]

@pointlessone I guess we need merge #1060 first here

[pointlessone]

Not sure. That one improves document consistency but I never saw a difference between 2.2 and 2.3+.

I'll leave it up to you. Wait for #1060 if you think it will fix the failure or you can dig and see if it's something worth checking before #1060.

[lucianosousa]

@pointlessone just merged and passes! :D ready for u

[pointlessone]

@lucianosousa Could you please clean up the history a bit?

[lucianosousa]

@pointlessone done

[pointlessone]

Thank you for your contribution.