Allow bigint annotations in rails 4. #569
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In rails 4, columns do not respond to `bigint?`. However, in both rails 4 and rails 5, columns do respond to `sql_type`. This way, annotations should work in both versions.
`bigint(8)` is a bit redundant. Can just use bigint.
|
@ctran I would appreciate any suggestions on this one. Thanks! |
ctran
reviewed
Aug 24, 2018
lib/annotate/annotate_models.rb
Outdated
| @@ -362,7 +362,7 @@ def get_index_info(klass, options = {}) | |||
| end | |||
|
|
|||
| def get_col_type(col) | |||
| if col.respond_to?(:bigint?) && col.bigint? | |||
| if col.sql_type == 'bigint' | |||
There was a problem hiding this comment.
There seems to be some logic behind bigint?.
I'd change it to:
if (col.respond_to?(:bigint?) && col.bigint?) || /\Abigint\b/.match?(col.sql_type)
This way we'll pick up any logic from activerecord if it's implemented.
Thanks for the PR.
There was a problem hiding this comment.
Great call! Relying on the existing logic is much better than writing our own match.
| @@ -183,7 +184,7 @@ def mock_column(name, type, options = {}) | |||
| [ | |||
| mock_column(:id, :integer), | |||
| mock_column(:integer, :integer, unsigned?: true), | |||
| mock_column(:bigint, :integer, unsigned?: true, bigint?: true), | |||
| mock_column(:bigint, :integer, unsigned?: true, sql_type: 'bigint'), | |||
| mock_column(:float, :float, unsigned?: true), | |||
There was a problem hiding this comment.
Can we test both of these?
- mock_column(:bigint, :integer, unsigned?: true, bigint?: true) # when bigint? is implemented
- mock_column(:bigint, :bigint, unsigned?: true) # when it's not
When using rails 5, it should use the logic already defined by ActiveRecord. When using rails <5, we should use very similar logic. Add spec to test for both cases.
|
Thanks for the feedback @ctran! Updated with both changes |
ctran
approved these changes
Aug 28, 2018
ctran
approved these changes
Aug 28, 2018
ctran
approved these changes
Aug 28, 2018
|
Thanks!!! |
mgpnd
pushed a commit
to mgpnd/annotate_models
that referenced
this pull request
Feb 15, 2019
In rails 4, columns do not respond to `bigint?`. However, in both rails 4 and rails 5, columns do respond to `sql_type`. This way, annotations should work in both versions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In rails 4, columns do not respond to
bigint?. However, in both rails4 and rails 5, columns do respond to
sql_type. This way, annotationsshould work in both versions.
Very open to suggestions in specs! Should I change
mock_column's signature tomock_column(name, type, sql_type, options ={})?If the backstory is helpful, bigint annotating was originally added in #515.