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

Additional README updates #563

Merged
merged 13 commits into from
Nov 14, 2018

Conversation

dillonwelch
Copy link
Contributor

Followup to #560, #556, and, #554 in the series to address #397.

Closes #430.
Closes #507.

Migrates the following wiki articles into the README:
https://github.com/zdennis/activerecord-import/wiki
https://github.com/zdennis/activerecord-import/wiki/Counter-Cache-Column
https://github.com/zdennis/activerecord-import/wiki/Examples
https://github.com/zdennis/activerecord-import/wiki/How-to-run-the-tests
https://github.com/zdennis/activerecord-import/wiki/Note:-ActiveRecord-Timestamps
https://github.com/zdennis/activerecord-import/wiki/On-Duplicate-Key-Ignore
https://github.com/zdennis/activerecord-import/wiki/On-Duplicate-Key-Update
https://github.com/zdennis/activerecord-import/wiki/Supported-Database-Adapters

I did not add in the Benchmark Wiki as the results seemed old (Jan 2017), specific to a particular DB, and did not have the script used so could not be validated.

Since we now have the majority, if not everything, in the README, now would be a good time to review and read the document as a holistic item and suggest changes to it as such.

README.markdown Outdated

### Array of Hashes

Due to the counter-intuitive behavior that can occur when dealing with hashes instead of ActiveRecord objects, `activerecord-import` will raise an exception when passed an array of hashes. If you have an array of hash attributes, you should instead use them to instantiate an array of ActiveRecord objects and then pass that into `import`.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkowens @ahmad-sanad I want to confirm that I understood the issue here correctly.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So an array of hashes does work, but only if the columns are consistent in every hash in the array.
e.g.

MyModel.import [{foo: 1}, {foo: 2, bar: 3}] # raises an exception

MyModel.import [{foo: 1, bar: 2}, {foo: 3, bar: 4}] # works fine

The potential workarounds discussed were:

  1. Convert it to an array of ActiveRecord objects
# Instead of this
MyModel.import [{foo: 1}, {foo: 2, bar: 3}]

# Do this
MyModel.import [MyModel.new(foo: 1), MyModel.new(foo: 2, bar: 3)]

# but hopefully with cleaner code lol
  1. Divide the array into multiple ones with consistent columns and import each one separately
# Instead of this
MyModel.import [{foo: 1}, {foo: 2, bar: 3}] 

# Do this
MyModel.import [{foo: 1}] 
MyModel.import [{foo: 2, bar: 3}] 

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, thanks for adding this!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NP! Thanks for the clarification. I will fix this up.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in regards to no. 2 the thought was to make use of Enumerable's #group_by.

For example:

arr = [{foo: 1}, {foo: 2, bar: 3}] 
arr.group_by(&:keys).each_value do |v|
 MyModel.import v
end

I think this would be the recommended workaround, but I don't have any benchmark data to support that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated hash examples again with group_by @jkowens

@@ -179,6 +494,28 @@ For more information on activerecord-import please see its wiki: https://github.

To document new information, please add to the README instead of the wiki. See https://github.com/zdennis/activerecord-import/issues/397 for discussion.

### Contributing
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkowens not sure if there's any other info here you want to add


```

Using hashes will only work if the columns are consistent in every hash of the array. If this does not hold, an exception will be raised. There are two workarounds: use the array to instantiate an array of ActiveRecord objects and then pass that into `import` or divide the array into multiple ones with consistent columns and import each one separately.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkowens @ahmad-sanad moved original answer to here

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me, thanks!

@jkowens
Copy link
Collaborator

jkowens commented Nov 6, 2018

@oniofchaos thanks for all the work you've put into this documentation! I was under the weather last week, so I'm just getting caught up. I'll try to review soon and if all is good I'll get it merged in. 👍

@jkowens jkowens merged commit d6c5871 into zdennis:master Nov 14, 2018
@dillonwelch dillonwelch deleted the additional-readme-updates branch November 14, 2018 23:04
@jkowens
Copy link
Collaborator

jkowens commented Nov 14, 2018

Thanks again for putting this together, it gives us a great framework moving forward. I agree now that we have largely everything moved the the readme it will be easier to see where we can fill in more details or even make some things more concise.

@dillonwelch
Copy link
Contributor Author

I'm glad I was able to contribute to this gem. I use it all the time and it was nice to give back.

@jkowens
Copy link
Collaborator

jkowens commented Nov 15, 2018

That's how it started for me too 😄

dillonwelch added a commit to dillonwelch/activerecord-import that referenced this pull request Nov 19, 2018
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.

3 participants