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 should not default to _id.to_s #261

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

Conversation

johnnyshields
Copy link
Member

#slug should not default to _id.to_s. This is inconsistent with the rest of the gem:

  • _slugs does not contain _id.to_s
  • clear_slug! sets _slugs to [], so #slug will return nil in this case
  • it will be overwritten when saving
  • before saving, one would expect both slug and _slugs to be nil

…est of the gem:

- _slugs does not contain _id.to_s
- clear_slug! sets slugs to [], so #slug will return nil in this case
- it will be overwritten when saving
- before saving, one would expect both slug and _slugs to be nil
@dblock
Copy link
Collaborator

dblock commented Aug 9, 2021

Ooooof I know for sure that Artsy relies on the fact that slugs default to ID, and aren't nil. This is a massive breaking change, isn't it?

@johnnyshields
Copy link
Member Author

johnnyshields commented Aug 9, 2021

@dblock we should make this behavior configurable. As noted above, its inconsistent. A "never touched" new model will have slug default to ID, but once you call clear_slug! the slug value becomes nil. I can investigate further on a way to preserve the behavior for Artsy.

@dblock
Copy link
Collaborator

dblock commented Aug 10, 2021

@dblock we should make this behavior configurable. As noted above, its inconsistent. A "never touched" new model will have slug default to ID, but once you call clear_slug! the slug value becomes nil.

I actually don't think it's inconsistent, but can be misinterpreted. Don't you want to clear the slug on clear_slug!? Maybe we need a clear_slug_and_set_to_default!?

I can investigate further on a way to preserve the behavior for Artsy.

Sounds good. It should be backwards compatible or we need to bump major and document the breaking change, all while allowing the functionality to exist.

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