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

#152: Breaks validates :uniqueness => true #153

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@recurser
Contributor

recurser commented Sep 11, 2011

Hey guys, this fixes the uniqueness validator problem as far as I can tell.

Thanks for all your hard work - much appreciated!

@norman

This comment has been minimized.

Show comment
Hide comment
@norman

norman Sep 13, 2011

Actually I think we can just change the condition in line 35 to <= rather than < and we'll cover this same condition without an extra conditional branch.

norman commented on lib/friendly_id/object_utils.rb in 7a5b9d8 Sep 13, 2011

Actually I think we can just change the condition in line 35 to <= rather than < and we'll cover this same condition without an extra conditional branch.

This comment has been minimized.

Show comment
Hide comment
@recurser

recurser Sep 13, 2011

Owner

Thanks, I've learnt something today :) yep please change to <= ... didn't realise that would work.

Owner

recurser replied Sep 13, 2011

Thanks, I've learnt something today :) yep please change to <= ... didn't realise that would work.

@norman

This comment has been minimized.

Show comment
Hide comment
@norman

norman Sep 13, 2011

Why did you make this change in so many places? This changes the semantics subtly, because now implicit block arguments will not be passed into super. The tests you added pass without this change. If you know for a fact that this is needed then let's add a test that shows that, otherwise let's not change this.

norman commented on lib/friendly_id/history.rb in 7a5b9d8 Sep 13, 2011

Why did you make this change in so many places? This changes the semantics subtly, because now implicit block arguments will not be passed into super. The tests you added pass without this change. If you know for a fact that this is needed then let's add a test that shows that, otherwise let's not change this.

This comment has been minimized.

Show comment
Hide comment
@recurser

recurser Sep 13, 2011

Owner

Hey norman, I added the (id) because I was under the impression that Hash or Array args such as {:name => 'joe'} wouldn't get passed to super without it - clearly I've missed a few ruby subtleties here. If the tests pass without it I don't have any reason to add it :)

Owner

recurser replied Sep 13, 2011

Hey norman, I added the (id) because I was under the impression that Hash or Array args such as {:name => 'joe'} wouldn't get passed to super without it - clearly I've missed a few ruby subtleties here. If the tests pass without it I don't have any reason to add it :)

This comment has been minimized.

Show comment
Hide comment
@recurser

recurser Sep 13, 2011

Owner
Owner

recurser replied Sep 13, 2011

@norman

This comment has been minimized.

Show comment
Hide comment
@norman

norman Sep 13, 2011

Owner

Thanks a lot for sending this. I had a couple comments in your pull request, could you take a look when you get a chance and then I'll pull?

Owner

norman commented Sep 13, 2011

Thanks a lot for sending this. I had a couple comments in your pull request, could you take a look when you get a chance and then I'll pull?

norman added a commit that referenced this pull request Sep 13, 2011

@norman norman closed this Sep 13, 2011

@workmad3

This comment has been minimized.

Show comment
Hide comment
@workmad3

workmad3 Sep 30, 2011

I also encountered this error. The problem appears to be something to do with the exists method that friendly_id overrides. I managed to 'fix' the error by removing the override here:
https://github.com/hedtek/friendly_id

This isn't a fix, just a hack to get around the issue temporarily. I'm posting it here to help other's attempt to isolate the root cause. (This was on rails 3.1.1.rc1, where the problem still seems to exist)

workmad3 commented Sep 30, 2011

I also encountered this error. The problem appears to be something to do with the exists method that friendly_id overrides. I managed to 'fix' the error by removing the override here:
https://github.com/hedtek/friendly_id

This isn't a fix, just a hack to get around the issue temporarily. I'm posting it here to help other's attempt to isolate the root cause. (This was on rails 3.1.1.rc1, where the problem still seems to exist)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment