search

SciRuby / daru

Data Analysis in RUby
BSD 2-Clause "Simplified" License
1.03k stars 139 forks source link

Issue 392 empty dataframe #395

Closed parthm closed 6 years ago

parthm commented 6 years ago

This patch allows Daru::DataFrame.new to create an empty dataframe as noted in issue #392. Comments / suggestions welcome.

rainchen commented 6 years ago

+1 for this feature, default value is neat solution.

But you should rebase your branch and there should be only 1 commit for this PR.

parthm commented 6 years ago

Good catch @rainchen . Rebased.

zverok commented 6 years ago

there should be only 1 commit for this PR.

It is not necessary, in fact. :) We use Squash&Merge github feature.

rainchen commented 6 years ago

@zverok "Don't make me think too much", actually this is a code review rule in my team. With clear commits can save reviewer's time. Reviewers can quickly to get the summary of a PR like "oh, there is 1 commit and 2 file changes and it's expected if I do this task by myself".

v0dro commented 6 years ago

You have a point. However, in this project where the only means of communication is a Pull Request and the issue tracker, having one PR per feature becomes a more generalized and easier way of communication. If there is one feature per commit, one needs to sit and read a list of commits, instead of quickly looking at the PR name and description. Its just more 'in your face' if you know what I mean.

Though it does not hurt to have one commit per feature in separate PRs :)

zverok commented 6 years ago

@rainchen Yep, @v0dro is right. Being one of the guys who does reviewing and merging, and considering we have people of various backgrounds trying to contribute, I explicitly state I am are OK with several commits per PR, not a big deal.