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

Slug will not be regenerated when owner object is updated #9

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dantler
Copy link
Collaborator

@dantler dantler commented Sep 11, 2013

I encountered a scenario where updating my class would cause the slug to be regenerated. This became very painful when I had two identical slugs who were both being updated frequently, as each new slug would be generated with a bigger number-suffix ("slug-2" would become "slug-3" and so forth).

My fix is to change the circumstances in which a slug might be regenerated by altering the "#stale_slug?" method. I've added a spec test defining the bug I am trying to fix. "should not increment slug number when update doesn't affect slug generation." Using the old code for the #stale_slug? method, this test will fail.

stale = true
end

if (permanent_slug? && (slug.nil? || slug.empty?)) ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIRC, nil? || empty? is not an equivalent substitution for blank?. Don't we have blank? in the latest stable DM1? If IIRC, it comes from extlib or active_support.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's from active_support. I read somewhere that it's equivalent, but I can change it back to "blank?" for simplicity. If this is the only reason we need active_support, would there be benefit in keeping this line the way it is?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, I wouldn't introduce such a big dep just because of one method.

Though, blank? would be true for strings of spaces (like " "). On the other hand, empty? wouldn't. You most likely would like to have slug.strip.empty? here or something like that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep. Here's how ActiveSupport defines blank? for strings.

def blank?
  self !~ /[^[:space:]]/
end

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