Suggestion for to_simple_form_partial_path

[asgerb]

Hey @tomasc, I have an idea for a small change, but wanted to throw it your way first, before making a PR 😉

My suggestion is to get the attachment's form partial path (to_simple_form_partial_path) by way of model_name, instead of relying on to_partial_path.

I often find myself overriding to_partial_path on my attachments, for various reasons. As we are currently relying on to_partial_path to get the partial for simple_form_attachments, this means I need to have the same directory structure here as well. Although this works fine, it somehow feels a bit strange to me.

Basically I think we could change this:

def to_simple_form_partial_path
  File.join('simple_form_attachments', to_partial_path)
end

To this:

def to_simple_form_partial_path
  File.join('simple_form_attachments', model_name.collection, model_name.element)
end

And get a more consistent naming/directory structure in return. What do you think, does it make sense?

[tomasc]

Hi @asgerb, sounds good in theory – could you please include a few examples of what kind of paths you are looking for in the end so that I can better see what would that mean in my case? Thanks.

[asgerb]

Sure! My attachments' partials are put in an attachments subdirectory in the views folder like this:

app/views/attachments/image_attachments/image_attachment
app/views/attachments/audio_attachments/audio_attachment
app/views/attachments/pdf_attachments/pdf_attachment
...

But this requires me to follow the same structure in the simple_form_attachments folder, where the added attachments seems redundant:

app/views/simple_form_attachments/attachments/image_attachments/image_attachment
app/views/simple_form_attachments/attachments/audio_attachments/audio_attachment
app/views/simple_form_attachments/attachments/pdf_attachments/pdf_attachment
...

[tomasc]

Strange, in my apps, I have the simple form partials always like this:

/app/views/simple_form_attachments/attachment_images/_attachment_image.html.slim

How comes that your paths are generated differently?

[asgerb]

Since we are relying on to_partial_path and I override that, sorry I should have explained that 😉

[tomasc]

Isn't it because your attachment or controller is namespaced? Attachment::ImageAttachment?

[asgerb]

It is happening with non-namespaced ones too.

[tomasc]

Ah I see, sorry!

[asgerb]

I could potentially see other times where you would override the to_partial_path for various reasons, but then it affects the partial lookup for simple_form_attachments too, which seems a bit strange somehow.

[tomasc]

Would not then be better to re-implement it like as here: https://github.com/rails/rails/blob/v5.1.4/activemodel/lib/active_model/conversion.rb#L94

def to_simple_form_partial_path
  File.join('simple_form_attachments', self.class._to_partial_path)
end

?

[asgerb]

Hmmm, perhaps, but how is this different from the current call to to_partial_path? Perhaps the way for me to solve this is simply to override to_simple_form_partial_path in the end.

[tomasc]

Well, if I understand correctly, you override instance method to_partial_path, correct? And your issue then is that the to_simple_form_partial_path takes that overridden method into account, while you don't want it to. So propagating the path generation to class by calling self.class._to_partial_path would solve your issue.

Still I wonder whether we should even do this. What we try to do in SimpleFormAttachments is to state there is relation between your model's to_partial_path and how the upload partials are structured. If you wish to stray away from that convention, then you simply override the to_simple_form_partial_path?

[asgerb]

Ah yes of course, hmmmm. That would solve my issue. But perhaps you are right in the end. I will simply override to_simple_form_partial_path 👍

[tomasc]

OK, let's keep this idea in mind in case more of us run into this problem though.