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

Fix blank slugs #571

Merged
merged 3 commits into from
Jul 2, 2014
Merged

Fix blank slugs #571

merged 3 commits into from
Jul 2, 2014

Conversation

xymbol
Copy link
Collaborator

@xymbol xymbol commented Jun 13, 2014

No description provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling a13bc79 on xymbol:fix-blank-slugs into aade1af on norman:master.

@norman
Copy link
Owner

norman commented Jun 13, 2014

@xymbol Thanks for working on this! I'm wondering if we need to bump a version for this, given the change in how slugs are generated, or would this be considered only a bug fix. Any thoughts?

@parndt
Copy link
Collaborator

parndt commented Jun 13, 2014

Is there any harm in bumping a minor version?

@norman
Copy link
Owner

norman commented Jun 13, 2014

I don't think so, calling this 5.1 wouldn't be a big deal, I just want to make sure that we only do so if that correctly communicates the degree to which this PR changes the library.

@xymbol
Copy link
Collaborator Author

xymbol commented Jun 13, 2014

@norman @parndt Actually, I needed this change to better handle some edge cases I've found with conflict resolution for multiple candidates. If you're okay with it, I can push the other changes, then we can bump the minor version.

Adrian Mugnolo added 3 commits July 1, 2014 23:07
Make more consistent with test suite.
Filter out unwanted slug candidates from generator.
@xymbol
Copy link
Collaborator Author

xymbol commented Jul 2, 2014

@norman @parndt Hi, guys.

I'd need these reviewed and merged so that I can rebase #578. Thanks!

@xymbol xymbol mentioned this pull request Jul 2, 2014
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling 194a3e4 on xymbol:fix-blank-slugs into fdbe416 on norman:master.

@@ -202,6 +202,12 @@ def self.name
end
end

test "should sequence blank slugs without a separator" do
with_instance_of model_class, :name => "" do |record|
assert_match(/\A([a-z0-9]+\-){4}[a-z0-9]+\z/, record.slug)
Copy link
Owner

Choose a reason for hiding this comment

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

@xymbol Sorry for taking basically forever to get back to you on this. Just a minor thing - since Ruby 1.9 we have \h for hexadecimar characters, so perhaps this could be checked more tersely with /\A[\h-]{36}\z/ instead here and anywhere else in the tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@norman Sure, I would like to see that regular expression factored out somewhere. It's all over the place.

@norman
Copy link
Owner

norman commented Jul 2, 2014

@xymbol Merging this - thanks and sorry again for the long delay in responding. Let's get your other recent changes in and release everything as 5.1. Before that I think we need to do a few updates on the Changelog too.

norman added a commit that referenced this pull request Jul 2, 2014
@norman norman merged commit dd4af0f into norman:master Jul 2, 2014
@parndt
Copy link
Collaborator

parndt commented Jul 2, 2014

Fabulous!

@xymbol
Copy link
Collaborator Author

xymbol commented Jul 2, 2014

@norman I've done that already waiting for this, pushing to a branch so that you can review.

@xymbol xymbol deleted the fix-blank-slugs branch July 2, 2014 14:11
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.

4 participants