weppos / breadcrumbs_on_rails

A simple Ruby on Rails plugin for creating and managing a breadcrumb navigation.
https://simonecarletti.com/code/breadcrumbs-on-rails
MIT License
944 stars 188 forks source link

Extract SimpleBuilder to a separate file #40

Closed kuraga closed 11 years ago

kuraga commented 11 years ago

23 was closed but I think it's right that builders (there is one now - SimpleBuilder)

should be in separate files.

weppos commented 11 years ago

Can you explain the reason behind this choice?

kuraga commented 11 years ago

@weppos in description now.

weppos commented 11 years ago

Actually, there are two builders: Builder (the abstract class) and SimpleBuilder. There are a couple of reasons that retain me to merge this patch.

First, if we extract this file into simple_builder.rb, the original file will be called Breadcrumbs and will contain a Builder definition.

Second, this file breaks the convention Namespace::ClassName => namespace/class_name.rb. To make things right, we should create lib/breadcrumbs_on_rails/breadcrumbs/simple_builder.rb, not lib/breadcrumbs_on_rails/simple_builder.rb.

I'm not sure this simple class deserves all this complexity.

kuraga commented 11 years ago

@weppos yes, it's my error about path. Fixed.

I think that abstract class may be not in separate file. But class that is just an example (yeah, but standard way) should.

I have wrote the reason. Thanks :-)

weppos commented 11 years ago

I appreciate your time and your contribution, however I believe I will keep all into this file until the need arises to extract it. For example, I will probably agree to extract it if I'll include and additional builder.

kuraga commented 11 years ago

:) compromise :-) :+1: