Open goosys opened 2 weeks ago
Thank you for this. I hope I can remember things clearly, let's see what we can do here... :eyes:
sortable
optionFirst, you are also introducing the sortable
option, which is a separate concern. That would be best served in a separate PR.
By all means, it's really good that you are implementing sortable
(it's been requested before, see https://github.com/thoughtbot/administrate/issues/1314), but separate, smaller PRs are easier to review and reason about. I'll be happy to review that.
The solution you propose is different from the one that was lost when https://github.com/thoughtbot/administrate/pull/920 was merged, resulting in issue https://github.com/thoughtbot/administrate/issues/1586.
You propose an option :getter
to define how a field will be accessed. In contrast, the lost functionality allowed the creation of fields that didn't need special options or interfaces.
So for example, it was possible to have this:
## field/foobar.rb
require_relative "base"
module Administrate
module Field
class Foobar < Field::Base
def self.searchable?
false
end
def foobar
"This is a Foobar: #{resource.inspect}"
end
end
end
end
## views/fields/foobar/_collection.html.erb
<%= field.foobar %>
In my view, the broken functionality was simpler to use and I would prefer that to return.
Also, I see that your code would only work when when @data.nil?
. This could lead to unexpected behaviour, as it's perfectly normal for a non-virtual field to be nil
instead of having a value. In my mind, it goes "if it's nil, perhaps it's a virtual field".
In comparison, the original functionality established distinction between virtual and non-virtual fields. In my mind, it goes "it is virtual or it is not, but there's no confusion".
I hope that makes sense.
What do you think? Would you be able to separate the sortable
code into a different PR, and change this one to behave the way I describe?
@pablobm Apologies for the confusion. With the implementation of virtual fields, the issue with the sort buttons on the index screen became more noticeable, so I initially included both in the same PR. I've now split the PRs. I'll address the points you mentioned later, and I'd appreciate your review again at that time.
@pablobm
Also, I see that your code would only work when when @data.nil?. This could lead to unexpected behaviour, as it's perfectly normal for a non-virtual field to be nil instead of having a value. In my mind, it goes "if it's nil, perhaps it's a virtual field".
This is for compatibility reasons.
We can fetch data exclusively from within the field using read_value
. While the application will work perfectly in this state, many RSpec tests fail because they insert data using the data
argument.
module Administrate
module Field
class Base
def initialize(attribute, _data, page, options = {})
@attribute = attribute
@page = page
@resource = options.delete(:resource)
@options = options
@data = read_value
end
Should I fix all the RSpec tests?
In that case, the data
argument in initialize
method would become unnecessary, so it would be better to either remove it or make it optional. However, this might cause compatibility issues with third-party Fields.
@pablobm
You propose an option :getter to define how a field will be accessed. In contrast, the lost functionality allowed the creation of fields that didn't need special options or interfaces. In my view, the broken functionality was simpler to use and I would prefer that to return.
I believe this case should not be an issue. For custom fields, you can override read_value to fetch data and format it using any method you prefer. I've written a sample and committed it, so please check it.
Support for virtual fields, based on #1586.
Usage
(Note: The Sortable implementation has been split into #2659.)
What do you think? I'll make any necessary changes. Please review it.