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

Make all Sequel::Inflections' methods public #1070

Closed
wants to merge 1 commit into from
Closed

Make all Sequel::Inflections' methods public #1070

wants to merge 1 commit into from

Conversation

kuraga
Copy link

@kuraga kuraga commented Aug 18, 2015

Make all Sequel::Inflections' methods public.
I.e. we can use Sequel.inflections.pluralize in plugins, code, etc.
Why are they private?

@jeremyevans
Copy link
Owner

As a general principle of API design, methods should be private unless there is a need to call them by external objects. The problem with this change is that would automatically add these methods to the public API of Sequel::Model (e.g. Sequel::Model.pluralize). I don't think these should be part of the public API of Sequel::Model, they are an implementation detail.

One option would be to change private to module_function. If we do so we need to make sure that all of these methods on Sequel::Inflections are tested, since they would become part of Sequel's public API at that point. I'm not sure I like that approach. The inflection methods are an implementation detail to the working of Sequel::Model, they are not designed to be used by external code.

The inflector extension offers a publicly available inflector, but it modifies a core class (String) and is only included for backwards compatibility. It's possible we could add another extension that just made Sequel::Inflections methods public (using module_function), but I don't really want to encourage direct use of the inflector in new code.

It's easy to increase the scope of the public API, it's much more difficult to decrease the scope. For that reason public API additions should be heavily scrutinized.

Can you open up a thread on the sequel-talk Google Group and see what the community thinks about this? I'm not against the module_function approach all that strongly, so we can certainly go that route if the community is in favor.

@kuraga
Copy link
Author

kuraga commented Aug 18, 2015

@jeremyevans, thanks for reply!

I just don't understand why "The inflection methods are an implementation detail to the working of Sequel::Model" if these methods don't use underlying Sequel::Model layer (cause they use received arguments instead)?

@jeremyevans
Copy link
Owner

Sequel::Model usesSequel::Inflections, not the other way around, see https://github.com/jeremyevans/sequel/blob/master/lib/sequel/model/base.rb#L4

@kuraga
Copy link
Author

kuraga commented Aug 18, 2015

Ok, in other words: methods of Sequel::Inflections (even extended into Sequel::Model) don't use self. They don't rely on object's state. They are, well, "static" (in terms of C++). Are "static" methods implementation detail and should they be private?

@jeremyevans
Copy link
Owner

The purpose of Sequel is to offer database access and an ORM (Sequel::Model), not for inflecting strings. The inflections are definitely an implementation detail related to the ORM side of things.

@kuraga
Copy link
Author

kuraga commented Aug 18, 2015

Thanks!

P.S.: The code when they use this API - here.

@kuraga
Copy link
Author

kuraga commented Aug 18, 2015

The last question (I'm sorry very much for discussion here).

Why do we extend Inflections but not extend Inflections; include Inflections? Then I can use inflection methods in Sequel::Model's eigenclass but not from Sequel::Model's objects...

class Post < Sequel::Model
  constantize('Post') # ok

  def meth
    constantize('Post') # undefined method error (on meth call)
  end
end

What's the sense here? Thanks.

@jeremyevans
Copy link
Owner

In general the inflection stuff is not needed at the instance level, only at the class level (automatic table naming, association method creation).

This is ruby, you are free to include Inflections in Model if it makes sense in your application. :)

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

Successfully merging this pull request may close these issues.

None yet

2 participants