Sinatra: what is the point of adding this?

[sushant12]

In the base.rb file what is the point of adding private?

[richiethomas]

@sushant12 what do you mean? Are you asking what the private keyword does, or why was it necessary to include it here?

[sushant12]

I am asking why was it necessary to include it there?

[sushant12]

private is only for methods right?

[richiethomas]

private can be for any chunk of code that shouldn't be public-facing or usable in other parts of the codebase. For example, the AcceptEntry class that you see immediately after the private keyword appears to be used only by the Request class, within this file.

My best guess is that the author's intent was to encapsulate a discrete set of logic inside of a class, but didn't want to make that class publicly accessible or instantiatable (sp?). However I just tried in irb and I'm able to instantiate it:

irb(main):001:0> require 'sinatra'
=> true
irb(main):002:0> Sinatra::Request::AcceptEntry.new("foo")
=> #<Sinatra::Request::AcceptEntry:0x007f9c5a378d10 @entry="foo", @type="foo", @params={}, @q=1.0>

If I'm right about their intent, I think the answer is to use private_constant, as per this article:

http://blog.arkency.com/2016/02/private-classes-in-ruby/

Good question, @sushant12 . I learned something new researching your question. :-)

[sushant12]

@richiethomas :) . Before this, I had never seen private used in that way beofre

[sushant12]

@richiethomas

With no arguments, sets the default visibility for subsequently defined methods to private. With arguments, sets the named methods to have private visibility.

Thats what I got from the api doc about private keyword but you said private can be for any chunk of code that shouldn't be public-facing or usable in other parts of the codebase can you please help me clarify?

[sushant12]

is private only for methods?

[sushant12]

same question for https://github.com/sinatra/sinatra/blob/master/lib/sinatra/base.rb#L650

[richiethomas]

@sushant12 I stand corrected! Looks like private is indeed only for methods. I had assumed otherwise because of the way I saw it being used in the sinatra/base.rb file. But the docs clearly back up your point (which would also explain why it didn't prevent me from instantiating AcceptEntry as I mentioned above).

At the risk of speculating, your question still stands- what is the purpose of using private here? The only thing listed under the private scope is that AcceptEntry class, i.e. no methods listed alongside it. So I'm not sure what it's meant to be "privatizing". There's even a test for a helper method which references the supposedly private class here:

https://github.com/sinatra/sinatra/blob/9bd0d40229f76ff60d81c01ad2f4b1a8e6f31e05/test/helpers_test.rb#L596

This implies that the class is meant to be something that can be instantiated, i.e. not a private class. I'm all out of ideas, but perhaps the core maintainers can clarify.

[sushant12]

any updates for this from the core maintainers would be great

[kgrz]

This private keyword is confusing, I agree ☹️ This is an artifact of code changes that happened overtime. The original change that introduced this is from https://github.com/sinatra/sinatra/commit/4f35139b594c6a56acff2f8ceb71000642518d6d

Notice that back then the private keyword was being used for a single method. When that method got changed, the keyword wasn't removed.

Reopening this issue so that we can track the cleanup. Thank you for pointing this out.

[sushant12]

Hi @kgrz , I have sent a PR :)

[zzak]

Sorry to have wasted your time here, the reason it is private is because _it is_.

We need a good reason for changing the visibility.

What is the use-case you have for making it public?

[sushant12]

@zzak but what is private being applied to?

[b264]

@zzak this could be a bug and I think you should reopen the @sushant12 pull request for more discussion. Perhaps private_constant :AcceptEntry after the class definition is what is actually intended here, but private is basically a code comment the way it's used here, and should be removed for clarity because it's not actually making the class private.

devbox@ubuntu-server-xenial:~/code/sinatra/lib$ bundle exec irb -r './sinatra'
irb(main):001:0> i = Sinatra::Request::AcceptEntry.new('foobar')
=> #<Sinatra::Request::AcceptEntry:0x005602d50982f0 @entry="foobar", @type="foobar", @params={}, @q=1.0>
irb(main):002:0> exit

Are you seeing something we are missing?

[mikegee]

I just came across this and I'm confused by @zzak's response too. These two occurrences of private don't change the visibility of the classes and modules that follow, so they are indeed useless.

I recommend reopening this issue and merging the associated PR.