platanus / potassium

A Rails application generator by Platanus, inspired by Suspenders
MIT License
232 stars 17 forks source link

Unify storage options #211

Closed difernandez closed 5 years ago

difernandez commented 5 years ago

Until now, the inclusion of active_storage or paperclip were two different asks. That meant that, theoretically, the user could enable both, which would result in unexpected behaviour. This PR unifies both under de storage key and the file_storage recipe

Changes

blackjid commented 5 years ago

Otra cosa seria buena agregar como dependencia la libreria que usemos para manejo de imagenes.. o no? Creo que es un caso muy comun. Para eso hay que agregar en el gemfile la gema image-processing. En el pasado hemos agregado imagemagick en en aptfile para que el buildpack lo agregue, pero tal vez es mejor usar https://github.com/minimagick/minimagick, que es una gema que agrega la libreria.... creo que es mas ordenado y mantenemos control sobre la version usando bundler no mas...

Paso adicional, seria evaluar https://github.com/libvips/ruby-vips, que es otra libreria de manejo de imagenes que claim que usar mucho menos memoria y es mucho mas rapido que usar imagemagick. Activestorage tiene soporte para esa libreria tambien.

https://edgeapi.rubyonrails.org/classes/ActiveStorage/Variant.html https://edgeguides.rubyonrails.org/active_storage_overview.html#transforming-images

difernandez commented 5 years ago

@blackjid se que se usa en varios proyectos, al menos ksec y pic-parks lo usan, pero proyectos nuevos no que yo sepa, quizás sería mejor eliminarlo (o al menos agregar un mensaje a la opción que indique que está deprecado). Había pensado en eso pero recuerdo que nos habíamos topado con problemas que si no me equivoco tenían que ver con ActiveStorage, como lo de las imagenes que a veces no se ven en refreshments (creo que tenía que ver con este issue). Por eso había pensado en mantenerlo, pero abierto a sacarlo si se cree que eso es mejor.

Con respecto al procesador de imagenes, tengo unas dudas:

blackjid commented 5 years ago
  • Hasta ahora potassium no se mete con nada de eso, ni con las gemas ni el aptfile no?

No, no lo hace, pero no se si es por decision de diseño o por que no hemos tenido la necesidad. Es algo que es bastante heroku depentiende, al menos la instalacion de apt.

Toda la razon

  • Esta feature de dejar instalado y funcionando un procesador de imagenes te lo imaginas como opcional (otro ask después de elegir storage) o como comportamiento por defecto? Pregunto porque no se bien que tan común es este caso de uso, dado que se necesita un storage

A mi me tinca que quede por defecto, pero en verdad no estoy seguro que tanto se usa.

Me tinca la idea de dejar paperclip con un [DEPRECADET] para desinsentivar su uso. En un siguiente release podemos matarlo.

Te tinca tiempo abrir un issue con el tema de imagemagic, vips, y el apt y tirarlo a discucion??

difernandez commented 5 years ago

Ya, agrego unos commits para dejar active_storage como default_value en las opciones y para agregar el mensaje de deprecado y redacto un issue para que discutamos lo del procesador de imagen

difernandez commented 5 years ago

@blackjid agregué el mensaje de deprecado y el default. Me queda una duda sí, el default sirve de algo cuando la pregunta es tipo flag? Me da la impresión de que solo serviría cuando es switch, porque ahí está la posibilidad de que el usuario presione Enter no más sin decir si/no

blackjid commented 5 years ago

Buena! No estoy seguro, puede ser que el default tenga más que ver con cuál es la primera que está seleccionada. Así uno puede apretar entre y se selecciona y funciona igual que para el switch

difernandez commented 5 years ago

@blackjid ya entonces lo dejo así por si acaso