Skip to content

Conversation

@randolf
Copy link

@randolf randolf commented Jan 16, 2019

Added a "separators" argument to the constructor to support customizable separators.

Updated the documentation to demonstrate and explain how the "separators" argument is used.

Added a "separators" argument to the constructor to support customizable separators.

Updated the documentation to demonstrate and explain how the "separators" argument is used.
Copy link
Owner

@zakame zakame left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Could you supply some additional tests for the newly exposed separators accessor as well? Also the current implementation is failing its tests...

has chars => ( is => 'rwp', init_arg => undef, default => sub { [] } );

has seps => (
has separators => (
Copy link
Owner

Choose a reason for hiding this comment

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

I think you can retain seps here and just remove its init_arg option so users can set it at object construction, e.g. Hashids->new( seps => 'aeiouy' ):

has seps => (
    is  => 'rwp',
    isa => sub {
        local $_ = shift;
        croak "$_ must not have spaces" if /\s/;
    },
    default => sub {
        my @seps = qw(c f h i s t u);
        join '' => @seps, map {uc} @seps;
    },
);

Copy link
Author

Choose a reason for hiding this comment

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

I'm fine with using "seps" rather than the longer "separators" keyword. I also like your suggestion for this preference because, now that I think of it, it can prevent problems resulting from a somewhat common misspelling that comes out as "seperators."


my @alphabet = split // => $self->alphabet;
my ( @seps, @guards );
my @seps = split // => $self->separators;
Copy link
Owner

Choose a reason for hiding this comment

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

Following the above, you won't need to split here...

for my $sep ( @{ $self->seps } ) {
# separators should contain only chars present in alphabet;
# alphabet should not contain separators
for my $sep ( @seps ) {
Copy link
Owner

Choose a reason for hiding this comment

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

... and instead do the split here.

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.

2 participants