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

Improve color regex #1021

Closed

Conversation

brendanthomas1
Copy link
Contributor

The current color regex uses [0-F] which allows all characters with a char code between 48-70. This would allow a user to pass in any of : ; < = > ? ` into their hex.

@@ -66,7 +66,7 @@
pdf.text ' '
text = PDF::Inspector::Text.analyze(pdf.render)
# If anything is rendered to the page, it should be whitespace.
text.strings.each { |str| expect(str).to match(/\A\s*\z/) }
expect(text.strings).to all match(/\A\s*\z/)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rubocop requested this

@@ -3,6 +3,7 @@
describe Prawn::Stamp do
describe 'create_stamp before any page is added' do
let(:pdf) { Prawn::Document.new(skip_page_creation: true) }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rubocop

@@ -26,6 +26,7 @@
let(:num_columns) { 5 }
let(:num_rows) { 8 }
let(:gutter) { 10.0 }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rubocop

@brendanthomas1
Copy link
Contributor Author

@pointlessone

Copy link
Member

@pointlessone pointlessone left a comment

Choose a reason for hiding this comment

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

👍

@pointlessone
Copy link
Member

@brendanthomas1 Thank you for your contribution. This PR will be merged a bit later when I have some time to work on Prawn.

@@ -94,7 +94,7 @@ def process_color(*color)
def color_type(color)
case color
when String
if color =~ /\A[0-F]{6}\z/i
if color =~ /\A[0-9|A-F]{6}\z/i
Copy link
Contributor

Choose a reason for hiding this comment

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

By adding a pipe here, you are allowing the pipe character too. The correct expression is:

/\A[0-9A-F]{6}\z/i

You can test this by running the following in irb:

/[0-9|A-F]/.match('|').inspect

vs

/[0-9A-F]/.match('|').inspect

Copy link
Contributor

Choose a reason for hiding this comment

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

The 'i' flag could be avoided by making it:

/\A[0-9a-fA-F]{6}\z/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @mojavelinux !

@pointlessone
Copy link
Member

@brendanthomas1 I cherry-picked your commit into master. Thank you for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants