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

Raise error for colors with leading # #869

Closed
wants to merge 1 commit into from

Conversation

tomprats
Copy link
Contributor

@tomprats tomprats commented May 5, 2015

Saw this issue and thought I could help! Really appreciate all the work you guys do!

@packetmonkey
Copy link
Contributor

Nice! Thanks for the PR!

First off please add an entry to the change log so we can doc the updated behavior.

Also I'm not sure that checking for the length of the string will be enough, as someone could say pass EEFFGG. What do you think about updating the logic to

when String
  if color =~ /\A[0-F]{6}\z/i
    :RGB
  else
    raise ArgumentError, "Unknown type of color: #{color.inspect}"
  end

Thanks again!

@tomprats
Copy link
Contributor Author

tomprats commented May 5, 2015

Good call! I wasn't thinking about the actual string content, I'll update both of those

@tomprats
Copy link
Contributor Author

I'm just going to close this because it's probably no longer valid

@tomprats tomprats closed this Feb 24, 2016
@packetmonkey
Copy link
Contributor

Hey I missed that you updated this branch, if you are still interested in getting it in I'm open to merging.

Thanks!

@tomprats tomprats reopened this Feb 24, 2016
@tomprats tomprats force-pushed the invalid-color-format branch 4 times, most recently from f4fd788 to cbf476b Compare February 24, 2016 19:38
@tomprats
Copy link
Contributor Author

@packetmonkey Thanks! Should be good to go now

@@ -10,7 +10,7 @@
Prawn::ManualBuilder::Example.generate(filename) do
stroke_axis

# Fill with Yellow using RGB
# Fill with Yellow using RGB (Unlink css, there is no leading #)
Copy link
Member

Choose a reason for hiding this comment

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

Unlike.

@tomprats tomprats force-pushed the invalid-color-format branch from cbf476b to 4858753 Compare February 24, 2016 21:12
@@ -94,7 +94,11 @@ def process_color(*color)
def color_type(color)
case color
when String
:RGB
if color =~ /\A[0-F]{6}\z/i
Copy link
Member

Choose a reason for hiding this comment

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

There are a few characters that we probably don't want to match between '9' and 'A'. I'd prefer this regex to be something more specific. For example /\A[0-9a-f]{6}\z/i.

@pointlessone
Copy link
Member

@tomprats If you're steal interested, please address my comment and rebase your branch on master. Thank you.

@pointlessone
Copy link
Member

@tomprats Than you for your contribution. I've rebased this PR on master and merged. GitHub didn't pick it up so I will close it now.

@tomprats tomprats deleted the invalid-color-format branch February 12, 2017 21:23
@tomprats
Copy link
Contributor Author

@pointlessone Thanks! Sorry I never got around to making the extra changes

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.

4 participants