Hi @filalex77, thanks a lot for your contribution!

Although you managed to lower duplication stats I'm not completely comfortable with this refactoring because at the end of the day the result is a generic (the classic "utility" module hintend by its nameHtmlHelper). The fact that you have to call the main entry point do_prefix_all_images (and the fact that the other two functions that used to be private are not private anymore) is also a hint to this issue.

For experience, generic sounding modules will end up becoming a repository of methods that we don't know where to put instinctively.

I think you're on a good path with the idea but the naming and placing need to be improved. The prefix_all_images method hints to its purpose in the first line which is a comment that says wrap with cloudinary or.... You also left img_of_size duplicated.

The helpers situation right now in the code base is not great, theoretically helpers should help view rendering, but we also have decorators and actual view objects so I can see why that's not very clear.

In this case prefix_all_images is a method used in a model so it's a little bit weird if it gets refactor in a view helper.

I'm not 100% sure yet where and how this code should get refactored into and i'm open for a discussion, I'm not convinced this is the best way though.

@lightalloy what do you think?

@rhymes Thanks! My thoughts exactly.

I put it as a helper for now to make discussion easier, but I was also mixed about where it should belong. And thanks for img_of_size, didn't notice it.

I'll wait on more input.

@rhymes, I agree, I'm not comfortable with the refactoring using helpers in this case too. I try to avoid adding more code to helpers unless it's a small change or a small method to use in the templates.
As one of the options to deduplicate I suggest:

• change Markdown#prefix_all_images to be a class method (that would require changing allowed_image_host, giphy_img and img_of_size to be class methods as well)
• change the code that uses this method accordingly
• call Markdown.prefix_all_images in HtmlVariant

This is not an ideal solution, of course, just a step to deduplicate.
Another option is to move the prefix image logic into its own class. Ideally, I would put the markdown parsing logic in its own namespace but it requires more work, so I wouldn't do it in this pr.

Hey @filalex77!

Thanks so much for the PR. With that said, we are in the process of going through and cleaning up some of our PR history to help us better prioritize what we need to be focusing on. We have decided unfortunately not to move forward with this PR so I am going to close it.

Thanks so much for being an awesome contributor! Ping us anytime if you have questions!