Purpose of checking current instance in every iteration of while loop?

Issue #21 resolved
scripted created an issue

If I understand correctly, these 2 lines make sure that the current instance is not already in the database.

    if instance.pk:
        rivals = rivals.exclude(pk=instance.pk)

Why don't we move this check before the while loop and return if instance.pk is not None (a valid instance.pk implies it's not a new object but an update operation) and hence there's no need to re-generate a unique slug. The while loop can be skipped in that case.

Comments (3)

  1. Andy repo owner

    Thanks for reading the code so thoroughly. The presence of a PK does not necessarily guarantee that the slug should not be regenerated because the user/app could reset it to an empty string or, even worse, replace it with another value which is already in the database. Still, there may be room for improvement. For example, we could track changes and only enter the while loop if either the PK is missing or the value of the field has been changed.

    By the way, I don't have much time for this project these days, so the best way to push an improvement into the trunk is to create a pull request with comprehensive tests (and documentation if needed). It's also preferable that the contributor adds his/her full name to the AUTHORS file.

  2. Log in to comment