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

Breaking change: Remove custom timestamp feature #861

Merged
merged 1 commit into from
Sep 6, 2016

Conversation

jaredbeck
Copy link
Member

@jaredbeck jaredbeck commented Sep 4, 2016

A little over four years ago, a feature was added to allow users to change the name of the versions.created_at column. See #129 There are three reasons why this should not be configurable:

  1. The standard column name in rails is created_at.
  2. The majority of the versions table is, and should be, an
    implementation detail of PT.
  3. This configuration option added moderate complexity to the
    library, and severe complexity to the test suite.

A little over four years ago, a feature was added to allow users
to change the name of the `versions.created_at` column. No reason
was given at the time.

#129

There are three reasons why this should not be configurable:

1. The standard column name in rails is `created_at`.
2. The majority of the `versions` table is, and should be, an
   implementation detail of PT.
3. This configuration option added moderate complexity to the
   library, and severe complexity to the test suite.
@jaredbeck
Copy link
Member Author

Andy and Ben, I've spoken with Jared M. and determined that this configuration option was added to allow companies to follow their own, non-standard, naming conventions.

Every other table in the database conformed to the same naming scheme for timestamp fields. .. we conformed to existing company database standards ..
#129

It's my opinion that companies building rails apps should follow rails naming conventions. The rails philosophy is "convention over configuration". So, for this and the other reasons I've given above, with the goal of simplifying PT, I recommend removing this configuration option.

As this is a breaking change, I'd appreciate your input. We may have different values when it comes to trading off simplicity vs. configuration. Thanks, everyone.

@airblade @batter @jrmehle

@airblade
Copy link
Collaborator

airblade commented Sep 5, 2016

@jaredbeck I agree with you.

@batter
Copy link
Collaborator

batter commented Sep 6, 2016

@jaredbeck - Your logic makes sense to me. I'm always in favor of conforming to convention and explicit / obvious patterns. I'm guessing this feature wasn't utilized often as I can't see an obvious use case beyond what @jrmehle stated for his company.

@jaredbeck jaredbeck merged commit cef21a7 into master Sep 6, 2016
@jaredbeck jaredbeck deleted the remove_timestamp_customization branch September 6, 2016 18:53
@jaredbeck
Copy link
Member Author

Thanks for the quick feedback, guys.

@jrmehle
Copy link
Contributor

jrmehle commented Sep 6, 2016

For the record, I am still completely against this change. This is a middle finger to anyone working with a legacy database (who may not always have the power to make the changes necessary). The fact that you or the maintainers of this project cannot see this and only want to remove the feature because it's hard to test is intensely disappointing.

@jaredbeck
Copy link
Member Author

I'm sorry, Jared. I totally understand how frustrating it must be. Every decision is a trade-off, and here we are trading off simplicity vs. configuration. It's not just to make our test suite simpler, the production code is simpler too. Another big part of our decision here is that we see the number of users who are unable to follow rails conventions as a very small segment of the population. If we're wrong about that assessment, obviously we'll have to reassess this decision. Sorry again, just trying to do what's best for the project.

jaredbeck added a commit that referenced this pull request Sep 8, 2016
See #861 for
discussion.
@jaredbeck
Copy link
Member Author

I've released 5.2.2 which includes a deprecation warning for timestamp_field= and mentions this discussion. If we find out that a lot (not a handful) of people need (not want) this feature, then we'll have to reconsider its removal. I hope this helps!

@jaredbeck
Copy link
Member Author

As of today, 2016-11-22, PT 5.2.2, which includes the deprecation warning, has 103 kilo-downloads. We have not heard any complaints about this deprecation. I think we're ready to release this breaking change (and others) as 6.0.

@airblade
Copy link
Collaborator

I agree.

@batter
Copy link
Collaborator

batter commented Nov 26, 2016

👍

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.

4 participants