Skip to content

Conversation

@jeromedalbert
Copy link
Contributor

@jeromedalbert jeromedalbert commented Dec 4, 2018

Q A
Bug fix? no
New feature? no
BC breaks? no
Related Issue Fix #263
Need Doc update no

Describe your change

This is the same code as PR #263 (which has been stale for more than a year), but fixed with all tests hopefully passing.

What problem is this fixing?

PR #263 used ActiveRecord::Version which doesn't exist: only ActiveRecord::VERSION exists. My PR fixes that.

The end goal of both this PR and the original PR is to use non-deprecated fields.

@jeromedalbert jeromedalbert changed the title Attribute changed deprecated Don't use deprecated attribute_changed? Dec 4, 2018
@jeromedalbert
Copy link
Contributor Author

jeromedalbert commented Dec 4, 2018

@julienbourdeau I think tests in this PR should be passing for all Travis build jobs, but I get this timeout error on random jobs:

https://keys.algolia.engineering/1/travis/keys/new: net/http: request canceled (Client.Timeout exceeded while awaiting headers)'

Tests pass on other jobs. Is this a temporary problem on your end? Or if these random errors are expected, is it possible to merge this PR?

Thanks!

@julienbourdeau
Copy link
Contributor

julienbourdeau commented Dec 4, 2018

@jeromedalbert Yes sorry about that. I'm looking into it, I dont understand why it's timing out 😭 It seems to happen a lot more in rails repo than other repo.

I think this is the most urging PR, so I think I'll release my first patch version tomorrow with it and release the rest of the PR/issues by batch.

@jeromedalbert
Copy link
Contributor Author

All checks pass! 🎉

@julienbourdeau julienbourdeau merged commit 3726d52 into algolia:master Dec 6, 2018
@julienbourdeau
Copy link
Contributor

Thank you for this contribution! 🚀

@jeromedalbert
Copy link
Contributor Author

jeromedalbert commented Dec 13, 2018

@julienbourdeau Thanks!

It looks like you minted v1.20.5 of this gem on GitHub, but it hasn't been pushed to RubyGems yet, where the latest version is 1.20.4.

So we're using master directly for now and haven't had any problems. Have a great Christmas

@jeromedalbert jeromedalbert deleted the attribute_changed_deprecated branch December 13, 2018 23:51
@vincentwoo
Copy link

Shipping this change without the necessary documentation changes was a huge mistake.

@julienbourdeau
Copy link
Contributor

It was, sorry @vincentwoo 🙏

@jeromedalbert If you can find time to review #338 I'd really appreciate your opinion on it.

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