Conversation
IO has been completely removed from lib/ and spec/ folders.
|
|
||
| RSpec.shared_examples 'correct macd' do |*args| | ||
| let(:source) { Daru::DataFrame.from_csv('spec/fixtures/macd_data.csv') } | ||
| let(:source) { Daru::DataFrame.read_csv('spec/fixtures/macd_data.csv') } |
There was a problem hiding this comment.
I somehow want to keep out the IO methods from the tests of daru. For example, a simple parsing error with daru-io could be misunderstood as a logical error with the method (of course, unless the error log is seen). Only 2 more fixture files are remaining - macd_data.csv and duplicates.csv. Can a truncated version of both these files be fed as manual data in the specs and then maybe remove both the files?
There was a problem hiding this comment.
Well, for me an ability to use CSV fixtures in general Daru tests seems important, especially for future (more complicated stats methods and stuff). So let's leave this as is.
And I believe that daru-io errors would be clearly seen in specs, if the problem is daru-io
| @@ -1,2 +1,3 @@ | |||
| source 'https://rubygems.org' | |||
| gemspec | |||
| gem 'daru-io', :git => 'https://github.com/athityakumar/daru-io.git' | |||
There was a problem hiding this comment.
Will daru-io be a runtime_dependency or development_dependency of daru?
There was a problem hiding this comment.
Runtime, of course. We need after gem install daru things like Daru::DF.from_csv to "just work".
athityakumar
changed the title
|
@zverok - Ready for a review 😄 Edit : I'll fix the merge conflicts while fixing the above review comments. |
zverok
left a comment
zverok
left a comment
There was a problem hiding this comment.
Please rebase from latest state of the branch (git pull --rebase upstream v-1-pre if remote sciruby/daru is named "upstream" on your machine), I've merged master and your PR shouldn't have unrelated
| @@ -1,2 +1,3 @@ | |||
| source 'https://rubygems.org' | |||
| gemspec | |||
| gem 'daru-io', :git => 'https://github.com/athityakumar/daru-io.git' | |||
There was a problem hiding this comment.
Runtime, of course. We need after gem install daru things like Daru::DF.from_csv to "just work".
lib/daru/vector.rb
Outdated
| # | ||
| # == Arguments | ||
| # | ||
| # * filename - Path of file where the vector is to be saved |
There was a problem hiding this comment.
Let's start to use proper YARD in all touched code. E.g. it should say
Save the vector to a file
@param filename [String] Path of file where the vector is to be saved
spec/dataframe_spec.rb
Outdated
| a = Daru::DataFrame.load(outfile.path) | ||
| expect(a).to eq(@data_frame) | ||
| end | ||
| end |
There was a problem hiding this comment.
Let's just remove Marshal totally. Nobody uses it, and nobody should.
|
|
||
| RSpec.shared_examples 'correct macd' do |*args| | ||
| let(:source) { Daru::DataFrame.from_csv('spec/fixtures/macd_data.csv') } | ||
| let(:source) { Daru::DataFrame.read_csv('spec/fixtures/macd_data.csv') } |
There was a problem hiding this comment.
Well, for me an ability to use CSV fixtures in general Daru tests seems important, especially for future (more complicated stats methods and stuff). So let's leave this as is.
And I believe that daru-io errors would be clearly seen in specs, if the problem is daru-io
daru.gemspec
Outdated
| spec.required_ruby_version = '>= 2.1.0' | ||
|
|
||
| spec.add_runtime_dependency 'backports' | ||
| spec.add_runtime_dependency 'daru-io' |
There was a problem hiding this comment.
@zverok - daru-io and daru are both runtime_dependencies of each other, and that's creating an infinite loop in travis builds while bundle installing. 
There was a problem hiding this comment.
Yeah, already thought of that. I believe that daru should be development dependency of daru-io (meaning since "daru 1.0" there is no sense in installing daru-io by itself, as it is installed with daru)
There was a problem hiding this comment.
Hmm, should v0.1.1 be released for daru-io with this dependency change?
There was a problem hiding this comment.
Probably so. At the same time when daru-1.0.0-pre would be released, probably?..
There was a problem hiding this comment.
Hm, but this way we can't continue developing daru in this branch :philosoraptor:
OK, let's leave this branch with development dependency on daru-io, and then change that on release.
There was a problem hiding this comment.
Acknowledged. Let me just add daru-io to the Gemfile rather than then gemspec, so that it doesn't interfere with travis CI builds. Before daru-1.0.0-pre release, this can be changed after daru-io-0.1.1 releases. Can you add this into the milestone?
|
Done. |
|
This needs a serious relook and separating IO from the core gem should be seriously considered before the next major release. |
3 participants
Through this PR, I'd like to safely port away all IO modules of Daru, keeping the Marshalling methods intact. I'll request a review in a couple of days.