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.

Sign up for GitHub

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
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7261215
Added spec tests to catch some corner cases, as well as fixes for them.
dantler Sep 5, 2013
48bdedc
Merge branch 'dev-env-fixes' into increment-fix
dantler Sep 10, 2013
e7293a5
Improved spec description
dantler Sep 11, 2013
23f201c
Cleaned up comments
dantler Sep 11, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 58 additions & 11 deletions lib/dm-is-slug/is/slug.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,18 +149,65 @@ def slug_source_value
self.send(slug_source)
end

# The slug is not stale if
# 1. the slug is permanent, and slug column has something valid in it
# 2. the slug source value is nil or empty
# 3. scope is not changed
# The slug is stale if
# 1. the slug is new
# 2. the slug is empty/has an invalid value
# 3. a property which affects the slug is dirty
# 4. scope change
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure that this comment adequately reflects the changed logic. Also, inverted logic is hard to evaluate. I'll give it a shot a bit later.

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 I flipped it to positive logic! I'm describing when "true" will be returned, rather than when "false" will be returned. To me this is easier to read, but I'm probably biased.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that listing cases when true is returned is better.

I'm saying that "the slug is new" doesn't seem to be inverse of "the slug is permanent, and slug column has something valid in it".

def stale_slug?
!(
(permanent_slug? && !slug.blank?) ||
slug_source_value.blank?
) ||
!(!new? && (dirty_attributes.keys.map(&:name) &
(self.class.slug_options[:scope] || [])).compact.blank?
)
stale = false
if new?
# 1. slug is new
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

(slug_source_value.nil? || slug_source_value.empty?)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This expression seem to be different to the original one.

# 2. Slug is empty and doesn't have a valid value
stale = true
end

return true if stale == true
Copy link
Collaborator

Choose a reason for hiding this comment

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

stale == true is a bit superfluous. Just stale would suffice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point! Curse my C background.


if (!permanent_slug? && false == dirty_attributes.keys.map(&:name).empty?)
# 3. Test for staleness. Does our dirty attribute change the slug
# source value? Lets do a test.
dirty_attributes.keys.map(&:name).each do |key|
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whoa, whoa! Hold on a sec here.

This is not a good way to do it. You can trigger hell of a side effect by all this assignments. If you really want to check it this way, you better cache slug_source_value after initialization (before any attributes got dirty) and compare it here with cached value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's also a bug when the attribute is stale because it's nil and dirty. This will claim it's not an important attribute and we won't know the slug has gone stale. Your idea is much better! I'll need to code that up...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you know a good hook which can be used to accomplish what you've described? #initialize might work but I think it would completely break lazy loading on the slug's generation properties.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bumping this line item. @cheba, do you know a solution to the "nil" problem I described, or can you help figure out how to accomplish the solution you suggested?

prev_value = self.slug_source_value
prev_key = self.send(key)

# Modify the information at :key
self.send "#{key}=", nil

# Test the slug source value for differences. This might
# outright fail due to us setting the property to nil, so
# lets call it stale when that happens.
begin
if self.slug_source_value != prev_value
stale = true
end
rescue
stale = true
end

# Restore key to what it was before.
self.send "#{key}=", prev_key

break if stale == true

end

end

return true if stale == true
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see why you resort to early return in this method. It cuts down unnecessary computation. Though, I really don't like it. You have to read the whole (now huge) method to understand why it returns not what you might expect.

I greatly prefer compact logic expession that was here before. It equally cuts down unnecessary computation and really is much more compact.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, the more compact evaluation felt like reading a regular expression to me. Hard to understand the whole thing without understanding how each piece evaluates. It's also really hard to set a breakpoint in the debugger or step through!

I prefer the spread-out style with comments next to what each operation represents. My reason is maintainability: adding an additional step to the algorithm or removing one becomes less complicated when each step is chunked by comments. With the old mathematical evaluation, you've gotta interpret each piece of logic and decide where a new one should be inserted!

I'm not too worried about extra computation. Computers process any O(1) algorithm quickly!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Computers process any O(1) algorithm quickly!

Unless it's IO-bound. 😉

Anyway, my concern is not that there's extra computation done. You effectively cut it down by early returns.

Ruby has implicit return. The last value of the method is what's returned. And most methods do just that. Early return breaks that pattern. That's why I don't like it much.

My suggestion. Break it down into smaller private methods and combine them here with logic operators.

def stale_slug?
  new? ||
  empty_slug? || invalid_slug? ||
  dirty_source? ||
  changed_scope?
end


unless (dirty_attributes.keys.map(&:name) &
(self.class.slug_options[:scope] || [])).empty?
# 4. Stale due to scope change
stale = true
end

stale
end

private
Expand Down
15 changes: 15 additions & 0 deletions spec/integration/slug_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,21 @@ class M::Thingy
post = Post2.first
post.slug.should == 'the-other-post'
end

it "should not increase slug number when updates are NOPs" do
2.times{Post2.create :title => 'The Post', :content => 'The content.'}
@post.update(:title => "The Post").should be_true
@post.reload
@post.slug.should == 'the-post'
end

it "should not increment slug number when update doesn't affect slug generation" do
2.times{Post2.create :title => 'The Post', :content => 'The content.'}
@post.update(:content => "Some content.").should be_true
@post.reload
@post.slug.should == 'the-post'
end

end

describe 'scoping' do
Expand Down