bogdan / datagrid

Gem to create tables grids with sortable columns and filters
MIT License
1.02k stars 115 forks source link

Fix `column(:attr, html: true, &:symbol)` #292

Closed robotdana closed 3 years ago

robotdana commented 3 years ago

rubocop told me to change column(...) { |record| record.value } to column(..., &:value) which should in most cases be equivalent. in this case, because datagrid does magic with arity and instance_exec it was raising instead. because the proc from Symbol#to_proc has -1 arity.

ActionView::Template::Error:
  no receiver given

I set the arity to 1 rather than passing the block e.g. data, row, grid

  1. because that solves this the Symbol#to_proc case: a instance_exec with more than one argument supplies the rest of the arguments the symbol method.

  2. because this is how non-html: true columns work currently work with negative arity. the receiver of instance_exec is the value supplied to the block, and no additional arguments are supplied

  3. because wanting the grid and row with splat arguments just seems unlikely

    column(:my_column, html: true) do |*args|
    # i can't think of anything here
    end

I also added a couple of already-passing tests because there didn't seem to be coverage of this case for non html: true columns or format values

bogdan commented 3 years ago

*args case can be a working case when you do some kind of delegation. Like:

def self.en_column(name, **options, &block)
  self.column(name, {order: false, html: true, optional: true, **options})) do |*args|
    I18n.with_locale(:en) { block.call(*args) }
  end
end

Is it hard to make *args case to work too?

robotdana commented 3 years ago

as far as i can tell the only way to distinguish between the symbol to proc case (which does something useful and expected with 1 argument and raises with argument error for more) and intentionally splatting arguments, was something along the lines of e.g. html_block.inspect.include?('&:') which i felt was too much of a hack for a pr.

I thought for a moment that ruby should have to_proc copy the arity of the method it's to_proc'ing but i realise the method doesn't necessarily exist and it doesn't know the receiver at the time (.eg. :to_s.to_proc can be called with symbol (with 0 args) or integer (with 1 optional arg (which is also -1 arity)).

The only other way of getting a useful arity out might adding arguments until you get argument error and going back one step or trying with them all and rescuing argument error and parsing 'wrong number of arguments, expected blah') but that seems even more hacky and likely to hide bugs.

robotdana commented 3 years ago

i double checked: existing behaviour (which this pr doesn't change) of non-html columns is 1:

it "should call all arguments for splat arguments" do
      rp = test_report do
        scope { Entry }
        column(:name) { |*splat| splat.length }
      end

      expect(subject.datagrid_rows(rp, [entry])).to match_css_pattern(
        "tr td.name" => "1"
      )
    end