Skip to content
This repository was archived by the owner on Nov 11, 2019. It is now read-only.

slug length counter bumps slug over max_length limit#254

Closed
codersquid wants to merge 2 commits intomasterfrom
title_length
Closed

slug length counter bumps slug over max_length limit#254
codersquid wants to merge 2 commits intomasterfrom
title_length

Conversation

@codersquid
Copy link
Contributor

When creating multiple videos with the same title, the unique counter appended to the slug bumps the entire thing over the max_length of 50 chars. related to #249

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hate magic numbers in code, but I'm not sure how to avoid it here. Is the comment good enough?

Copy link
Member

Choose a reason for hiding this comment

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

You can get the max_length this way:

Video._meta.get_field('slug').max_length

@willkg
Copy link
Member

willkg commented Sep 3, 2014

Another way to do this is to hash the date with sha1 encode it with base16 and then take the first x characters (7 or 8 is probably sufficient) and use that. It reduces the likelihood you have to do multiple iterations because it doesn't have the problem that incrementing a counter has. However, it uses 7 or 8 characters.

I think this is probably the way to go.

@willkg
Copy link
Member

willkg commented Sep 4, 2014

@codersquid If you don't want to work on this, I can finish it up. Either way is fine by me.

@codersquid
Copy link
Contributor Author

I'm closing this and making a pull request from another branch, see #269

@codersquid codersquid closed this Sep 5, 2014
@codersquid
Copy link
Contributor Author

I decided not to do the hash based on time because of collisions when doing batch creations -- the calls happen in under a second, and time.time() or datetime.datetime.now() don't have enough resolution. but I could be wrong! I searched a little just to see. Doesn't seem so. Instead of getting carried away and using a random seed and blah blah I went back to the loop thing.

@codersquid
Copy link
Contributor Author

oh and yeah you are right about the annoying corner cases with the counting approach.

@willkg
Copy link
Member

willkg commented Sep 9, 2014

Sorry for the lateness--I plan to review this sometime this week.

@willkg
Copy link
Member

willkg commented Sep 9, 2014

Whoops--wrong pr. :p

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants