remi icon indicating copy to clipboard operation
remi copied to clipboard

Follow Python's "Best of the Best Practices" more closely

Open cryzed opened this issue 8 years ago • 11 comments

I'm not a PEP 8 code-snob, promise! I'm specifically talking about the getter- and setter-methods which are more at home in other languages, which nearly all of your widgets make use of. Especially at what I would consider the beginning of the project, you have a chance to make the Widget code very pythonic without any drawbacks (i.e. breaking backwards-compatibility).

Maybe I am being shortsighted, is there a reason why you are simply not using properties or attributes (when there's no logic involved in setting and/or getting the attribute)?

PS: "Best of the Best Practices"

cryzed avatar Nov 12 '15 17:11 cryzed

Agree. I slowly port Remi to better practices and more PEP8 compliant as I change the code. There is much work to do, and I will accept patches for this.

nzjrs avatar Nov 12 '15 17:11 nzjrs

Awesome, sounds great! I'll see about possibly contributing patches in the future :).

cryzed avatar Nov 12 '15 17:11 cryzed

What setters / getters do you refer to specifically? the problem with set_onclick_listener and friends are not that they are setters per se, but rather that it feels foreign in gui frameworks, which would use a connect or bind verb widget.connect('onclick',function).

Are you referring to the widget.attributes dict?

If you can provide some more examples I can advise on which I feel are a the worst offenders.

btw: this is why it is not ready for pypi yet

nzjrs avatar Nov 12 '15 17:11 nzjrs

I agree that the methods that register listeners should definitely stay methods -- possibly renaming them to add_listener would make sense, since set_listener feel very singular, as if it's only possible to register a single listener, i.e. overwriting possibly previously registered listeners.

As for an example where it would make sense to use properties, check out the Button.set_text method, wouldn't it be just as easy to access Button.text? Same goes for TextInput, Label, etc.

Using set_value for the ListView widget on the other hand, makes of course sense -- since you are actually setting the selected value. The name might be a bit ambiguous, but I've always been very bad at naming things well. Maybe set_selected_value or select_value? I'm not sure, I like it the way it is currently.

cryzed avatar Nov 12 '15 17:11 cryzed

Personally I think property setters in python feel too immediate and opaque to me. Most code I admire does not use them a lot. Take the case of (html) attributes and text on a widget. They are very different things in terms of implementation and of immediacy of API contract. I think having both appear as properties is misleading as it gives them an appearance of similarity. For the defining characteristic of the widget (Button text, Label text) etc I think it is fine to have that an explicit function call.

nzjrs avatar Nov 12 '15 17:11 nzjrs

By opaque you mean that simply setting an attribute on the object might make the API less easily understandable, because it's not immediately clear that doing so will have an effect on the widget's state (as displayed on the website)? I suppose that's possible, and nothing one couldn't change easily if you ever decide otherwise.

I admit that I don't have much experience in API design in general and I'm not sure how convenient using Python's property decorator might be in the long run, I just figured it would make sense to use them, since this would certainly be an opportunity. But I think I get what you mean when you say it makes the code more opaque: possibly more divorced from the functionality and more "magicky" than it has to be? Unless I am misunderstanding?

cryzed avatar Nov 12 '15 17:11 cryzed

Nope, you understand my point. The most common application of property getters and setters I see is to fix previously public attributes.

I am wary of using them.

nzjrs avatar Nov 12 '15 17:11 nzjrs

Alright, I would have to agree then.

For something completely different then, regarding the naming of identifiers: You are using underscores in some methods, e.g. set_on_focus_listener but then some of your other methods are simply named onfocus -- what's the rationale behind that? (This is just nitpicking at this point, since most of my points have been ruthlessly shut down :P). Also since these methods are just used as internal callbacks that propagate the event to the registered listener, wouldn't it make sense to prefix these with underscores to make them "private", indicating to users that these methods are usually only used internally? I don't see an immediate reason why exposing these methods to the user would make sense.

cryzed avatar Nov 12 '15 17:11 cryzed

All the things you point out are on my TODO list and I disagree with none of them.

There is a lot of public API leaking here. I will take patches fixing any of them!

nzjrs avatar Nov 12 '15 17:11 nzjrs

It's good to hear that I'm not only making moot points. As I said previously, I'll try to see if I can possibly contribute patches in the future as well. Thanks for your quick replies!

cryzed avatar Nov 12 '15 17:11 cryzed

Hi, I would only say this. Some methods like onfocus have to be as they comes because they are managed with event automation easly, reflecting javascript event names. Take care about that. But however there are a lot of methods and attributes having names improperly defined (because of my mistake) that have to be changed.

dddomodossola avatar Nov 13 '15 01:11 dddomodossola