booglybob / pyglet

Automatically exported from code.google.com/p/pyglet
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

Make clock.update_time private. #689

Open GoogleCodeExporter opened 8 years ago

GoogleCodeExporter commented 8 years ago
clock.update_time is only used outside clock in the function EventLoop.idle, by 
calling 

dt = self.clock.update_time()
redraw_all = self.clock.call_scheduled_functions(dt)

which is in fact called in clock.tick().

update_time is an implementation detail of clock, is not documented and not 
tested, so I suggest putting it private (changing it to _update_time() ), such 
that it becomes part of clock private API.

Original issue reported on code.google.com by jorgecar...@gmail.com on 10 Dec 2013 at 8:11

GoogleCodeExporter commented 8 years ago
The changes for this are two lines in pyglet/clock.py:

def update...  -> def _update... 
and 
delta_t = self.update_time() -> delta_t = self._update_time()

and two lines in pyglet/base.py:

remove
272 dt = self.clock.update_time()
273 redraw_all = self.clock.call_scheduled_functions(dt)

and add 
272 self.clock.tick()

Original comment by jorgecar...@gmail.com on 10 Dec 2013 at 8:14

GoogleCodeExporter commented 8 years ago
update_time it is documented: 
http://pyglet.org/doc-current/api/pyglet/clock/pyglet.clock.Clock.html#pyglet.cl
ock.Clock.update_time

It's part of the class, I can't see why we want to hide it as in Python there's 
no real "private" or "public".

Can you elaborate what are the benefits of changing this method?

Original comment by useboxnet on 10 Dec 2013 at 8:45

GoogleCodeExporter commented 8 years ago
I'm not sure the reason should be on this ticket or on the mailing list, 
because is more conceptual.

You are right that in Python there is not strict enforcing private or public. 
However, that doesn't mean there is no real private or public. While the 
paradigm is "We're all consenting adults here", developers are encouraged to 
use an underscore prefix to tell *other developers* that a given function or 
attribute is private within the scope of the class. This means that, if the 
developer uses it, it is its own responsibility (and not responsibility of the 
developer of the API).
This is particularly relevant in deploying APIs since the end user of the API 
must know what is its interface. There are very good and well as documented 
reasons for this.

My point is, update_time is part of the private implementation of clock: it is 
not made to be used outside clock, but to be used within tick. If it was, first 
it would have to be tested, because any interface has to be tested; then, it 
would be used somewhere outside clock, which it is, but, as far as I was able 
to track, incorrectly. The very basic reason being that the EventLoop is using 
a function that is not tested.
Moreover, it is not documented. The link you sent is not a documentation as far 
as an interface is concerned; it is an API reference. Documentation of a public 
interface is the equivalent to the Programming Guide: it has to explain in 
which context that function has to be used, and not what it does.

Besides the benefit of putting a non-tested interface back to private, thus 
avoiding it to be used by Pyglet users without discretion, the less public 
functions any API has, the less complex it is because the less entry points it 
has. The API should have the minimum number of public functions required for 
doing what it says it does. In this case, as far as I understood, the interface 
is clock.tick(), not update_time.

If we don't reach a consensus, I suggest we shift the discussion to the mailing 
list.

Original comment by jorgecar...@gmail.com on 10 Dec 2013 at 9:19

GoogleCodeExporter commented 8 years ago
Thanks for your comment. It makes things easier if all the information is in 
the issue (although a link to the mailing list archive it's OK too).

We don't completely agree about what qualifies as documentation :), but I don't 
have a strong opinion about this method being public or not.

As you seem inclined to think we're not going to reach consensus, feel free to 
get feedback from the mailing list (I'm not the only one committer!) and please 
update the ticket so we have full history of the issue.

Thanks for the report!

Original comment by useboxnet on 10 Dec 2013 at 11:05