Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleaning up syntax #34

Open
gorn opened this issue Nov 13, 2017 · 6 comments
Open

Cleaning up syntax #34

gorn opened this issue Nov 13, 2017 · 6 comments

Comments

@gorn
Copy link

gorn commented Nov 13, 2017

I like Cells gem for Rails very much, the only thing I do not like it the syntax. For example I find

cell(:post,post).(:title_with_date)

very ugly. I respect, that the author has his own syntac preference, I just want to be able to make it more rubyish for my purposes. Alternative syntax

PostCell.(post).(:title_with_date)

is slightly better, but I would prefer something like

post.cell.title_with_date

or at least post.cell(:title_with_date). I tried to implement this in possibly very naive way:

class Post < ActiveRecord::Base
  ...
  def cell(method_symbol)
    PostCell.(self).(method_symbol)
  end
end

it works quite well for "not so comlicated" methods, however when I use it on cell method link_to_title which looks like this

class PostCell < Cell::ViewModel
  ...
  def link_to_title
    link_to model.title, model
  end
end

it fails with

undefined method `[]' for nil:NilClass
on line 58 of the gem code https://github.com/trailblazer/cells-rails/blob/master/lib/cell/rails.rb#L58

It seems to be missing some part of the context, but I do not know how to make it happy :(

@apotonick
Copy link
Member

Hi Jakub,

this has nothing to do with "syntax" but with semantics. Cells is not a DSL but a functional object that can be called and returns a HTML fragment. If you read the Trailblazer::Cell docs, you will see the intended new API will be even simpler.

cell(Post::Cell::Show, @post)

A cell is not an object that can be passed around and you call various methods on it, the times when this was considered good design have past long since. We want to minimize wrong usage of our gems, which automatically minimizes the public API. Something like

post.this.that.method

is very likely to be called in the wrong order, etc. If you need something like that, you will be better off with a simple decorator object.

Also, "ugly" is your subjective interpretation - Cells and all TRB gems don't want to be "idiomatic Ruby", because we think huge objects with a lot of behavior were a mistake, and your proposal unfortunately encourages a big public API (unless I misunderstood something?).

So, again, this is not about "the author's preferred taste", this is about enforcing good API design, a minimalistic public API and reducing errors.

@apotonick
Copy link
Member

Can you make some examples how you use Cells? Because I can see from your snippets that you call different "helpers" on one cell instance - which is not what Cells are designed for. Please paste some usage examples so I can understand the context better! 🍻

@gorn
Copy link
Author

gorn commented Jan 1, 2018

@apotonick thanks for your reply.

A cell is not an object that can be passed around and you call various methods on it, the times when this was considered good design have past long since.

Could you provide any link to learn more about this. It might that I am stuck in "past" from your point of view. Please take all my rants with this in mind. I am not suggesting post.this.that.method style, i am suggesting post.cell.method style. I do not need the methods being changeable, however I see no benefit in writing things like cell(Post::Cell::Show, @post) the main reason being that it is not very dry. The @post object already contains information that it is Post class which we are talking about here, so why we need to repeat it. The method cell already implies that we will need to reach for the Cell so why we need to write it again every single time. Something like cell(:show, @post) is the dryified version (having its own disadvantages). That is what I mean.

Also the syntax CommentCell.(@comment).() is very strange, the .() part sound more like some black magic, than good syntax :) + again it is not dry at all, it is obvious from @comment class that we will need CommentCell not it is nor very dry it also does not follow neither the principle of least surprise nor the "convention over configuration" and all of these principles are (for me at least) main stepping stones in Rails and Ruby

Also I do not intend to do it, it surprising to hear that "A cell is not an object that can be passed around and you call various methods on it" ... that is somehow against the nature of the object is not it? In ruby everything is object which is passed around and methods are called upon it :)

Cells and all TRB gems don't want to be "idiomatic Ruby", because we think huge objects with a lot of behavior were a mistake ...

I am very interested in learning about this point of view, can you point me to any resource which elaborates on this? Without more details I do not agree, but I am eager to try to understand. For now it seems to me that by trying to be "non idiomatic" you are introducing very strange syntax which will bring more confusion than benefits, but obviously it is just personal opinion.

Generally I really appreciate the idea of turning helpers/partials into cells which have its controller and view parts and I feel it greatly improves quality of my code. Thanks a lot. Here are examples of my code, but the usage is really simple

cells/guidepost/nazev_or_kct_nazev.erb

<i class="dimm"><%= guidepost.kct_guidepost.andand.nazev %></i>

cells/guidepost_cell.rb

class GuidepostCell < Cell::ViewModel
  property :nazev
  property :trails
  property :kctref

  def show; link_to guidepost end

  def nazev_or_kct_nazev
    nazev || render
  end

  def kctref_link
    kctrefs = guidepost.kctrefs
    label = case kctrefs.size
              when 0 then '?????'
              when 1 then kctrefs.first
              else kctrefs.first+'+'
    end
    link_to(label, guidepost)
  end
 def brief_links_to_related;  render end
end

which is than used in view like this

view/guideposts/index.html.erb

....
<table>  
  <thead>
    <tr><th>KČT ref</th><th>uka</th><th>Název</th>
    <th>Výška</th><th></th></tr>
  </thead>
  <tbody>
    <% @guideposts.each do |guidepost| %>
      <tr>
        <td><%= cell(:guidepost, guidepost).(:kctref_link) %></td>
        <td><%= cell(:guidepost, guidepost).(:brief_links_to_related) %></td>
        <td><%= guidepost.nazev_or_kct_nazev %></td>
        <td><%= guidepost.vyska %></td>
        <td><%= link_to 'detaily', guidepost %></td>
      </tr>
    <% end %>
  </tbody>
</table>

@apotonick
Copy link
Member

Your syntax is encouraging "fat cells" 😜 which basically means people will use one cell class for many different concerns, which, since you like to argue in pattern names, breaks SRP. They will write a lot of methods completely unrelated and push then into one class. As you pointed out, the current low-level syntax is indeed confusing when calling a cell, I agree the chain of .() is unnecessary. Please check the comment below.

However, I do not really understand why you mention "dry"_ all the time - you are not repeating anything here. The fact that you know that a @comment object wants a CommentCell is your human understanding of the domain, and you can easily build this magic yourself on top of the framework, but Cells doesn't know anything about ActiveRecord and will never do, so you have to specify the cell class name explicitly, which is what you're calling "principle of least surprise", because the framework is not guessing stuff for you and hence won't surprise you at some point in the future the way it happens to thousands of Rails devs the moment you're reading this. 🤣

To talk about your example: You want to use Cells as a partial replacement, and that's great and your idea is correct. In your case, the cell syntax looks odd, because you're a) not using Trailblazer::Cell, which will be the future structure/syntax in Cells 5, and b) because you're using one class for many "views", which is no longer the philosophy in Cells ("fat cells" haha). In Cells 5, one cell class does exactly one thing, which is representing one view fragment. I'd say, let's discuss how much we should "allow" your "fat cell" style or how much we should nudge developers to a clean SRP architecture.

@apotonick
Copy link
Member

BTW thanks for the example, it's a cool starting point to discuss! ❤️

@gorn
Copy link
Author

gorn commented Apr 10, 2019

I have mentioned DRY principle several times, because this is what surprises me most in your syntax. Why to write

cell(Post::Cell::Show, @post)

when the first part of the first argment (Post) should always be class of the second argument (@post). This information is repeated two times on the same line. Therefore you repeat yourself, which is not DRY.

Fat cells: Yes perhaps I tend to use it this way which you do not like, but I personally do not think that my usage is againsts SRP. I think you are taking SRP too extremely. In Ruby you have methods organized in classes and they are in turn optionally in modules. If you take SRP to extreme that you can say that any module should only contain one class with one method, which is somehow wasting what ruby offers. What I am trying to do is to have a Cell which represents all small decorators of a single object and methods represent indicidual decorators. You take it one level up and suggest that every Cell represents one decorator, so you will probably need to introduce one more organiyational level which contains all cells working with one model. This way you gain nothing, you are only shifting the hierarchy elsewhere, but there is no simplification. Therefore this seems to me as purely question of taste.

I am still interested in the comment you made in your first answer, where you are saying:

A cell is not an object that can be passed around and you call various methods on it, the times when this was considered good design have past long since.

Can you point me to a resource elaborating on this? I have never heard about this direction of programming evolution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants