Option for ignored columns on update #51
Conversation
|
The CI failure seems to be unrelated |
There was a problem hiding this comment.
Overall looks good to me.
The test suite in travis is not reliable anymore since we have too many versions how reached the end of life and we are missing the most recent.
May I ask to to update also the .travis.yml removing 2.2, 2.3 and adding 2.6 and 2.7 before ruby-head?
Thank you very much
lib/bulk_insert/worker.rb
Outdated
| @@ -16,12 +16,14 @@ def initialize(connection, table_name, primary_key, column_names, set_size=500, | |||
| @ignore = ignore | |||
| @update_duplicates = update_duplicates | |||
| @return_primary_keys = return_primary_keys | |||
| @ignored_columns_on_update = Array.wrap(ignored_columns_on_update) | |||
There was a problem hiding this comment.
@jamis I am not sure if the gem is supposed to work only with a dependency on activerecord or it was designed to have a dependency on activesupport as well:
There was a problem hiding this comment.
Ah, good spot. I don't think we need to keep this, and can use Array(ignored_columns..) as we are not expecting anything unusual in that param like a hash.
aqeelvn
force-pushed
the
option-for-ignored-columns-without-default
branch
from
9460dbe to
d68520a
Compare
|
Thank you for taking a look @mberlanda, appreciate it. I've applied your suggestions. Could you take another look when you get a chance? |
aqeelvn
force-pushed
the
option-for-ignored-columns-without-default
branch
from
b6243db to
fa25dd0
Compare
|
@mberlanda Thank you. Have ignored |
|
I would wait for @jamis feedback before merging. I was considering to go for a major bump dropping the support for ruby < 2.4 and rails < 5. |
|
Hi @jamis , just wanted to quickly check if you had a chance to take a look at this one. Would appreciate any feedback you have. |
|
@aqeelvn I hope you are doing great! Would you be available to re-submit your changes in a new revision of this PR rebasing on master? Thank you very much for the time you spent on this contribution |
|
hey @aqeelvn do you want me to cherry-pick your changes and adapt them to the new gem structure? |
|
@mberlanda Hey, thanks for reaching out. Yes it would be good to get the changes one way or the other. I know @sikachu were trying to get my changes on the latest master few days ago. @sikachu are you still working on it? If not it would be great if you can cherry-pick my changes as you suggested, sorry I'm going through a busy schedule lately. |
|
@aqeelvn I'm also focusing on something else right now so @mberlanda please feel free to work on this. |
Closes #50
Alternately I also noticed #49 which also attempts to fix the same issue but the API interface is slightly different.
I could be wrong, but I'm guessing typically you may want to avoid only a small number of columns on
UPDATEso rather than having to explicitly define all the columns it might be more convenient to just specify those columns that need to be ignored.Also this interface would nicely allow for having convenient default values like
created_at. I haven't added it though in this PR as that could potentially be a breaking change and may need a major version bump depending on what the maintainers think.cc @jamis @mberlanda