Skip to content

Conversation

@gee-forr
Copy link

Hi there,

I came across an issue where I couldn't validate codes longer than 3 parts. Turns out, it wasnt documented, and I just needed to add the parts as a second positional argument.

This however is inconsistent with the interface for the generate method.

I've updated the validate method's arguments to match those of generates, and have also added a feature to set a default parts length.

I've also updated the readme (but only in english, I don't speak Korean unfortunately), and bumped the version.

I hope you find the changes to your liking :) Thanks again.

Choose a reason for hiding this comment

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

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

Choose a reason for hiding this comment

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

Unnecessary spacing detected.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

Choose a reason for hiding this comment

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

Unnecessary spacing detected.

@baxang
Copy link
Owner

baxang commented Sep 13, 2015

Thanks for contributing @gee-forr. Sorry for being late. I've been sick for the last couple days 😢

As you pointed out, I agree that those #generate and #validate's method signatures are not consistent and it should be addressed.

However I'd prefer to leave current interface as it is because changing #validate's argument doesn't seem to improve the library's usability significantly. Instead, I am working on adding configuration so that the users of this gem can change the number of parts, the length of each part, etc.

The end result will look like:

CouponCode.configure do |c|
  c.parts = 5
  c.length = 6
  c.separator = '-'
end

I believe this approach will provide with more convenient use cases.

You can find working in progress code at #6. Please feel free to create another PR for other implementation suggestions.

@baxang
Copy link
Owner

baxang commented Apr 8, 2016

I updated README to make that multi-part validation clearer for now: 0d40bb4. Thanks again for your time and effort and my apology for being late 😭 I will be adding configuration as soon as possible.

@baxang baxang closed this Apr 8, 2016
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.

3 participants