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

Method naming scheme #235

Closed
mpmc opened this Issue Sep 1, 2017 · 31 comments

Comments

Projects
None yet
3 participants
@mpmc
Contributor

mpmc commented Sep 1, 2017

When writing classes, I try to do method names as simple as possible.. e.g.

def do():
   return value
def do_set(val):
    value = val

or when I group methods..

def main_foo():
    pass

def main_bar():
    pass

I noticed that appJar is using CamelCase, with add/set etc at the start of method names, is there a reason for the non pep-8 usage? I'm not saying this way is wrong, I just find it harder to read.

For example addLabelEntry/addLabelNumericEntry could be replaced with entry() and numeric_entry(), To get values you could just simply use entry_get() which for me is a lot easier to read and avoids camels.

All this of course is a matter of personal taste.

@jarvisteach

This comment has been minimized.

Owner

jarvisteach commented Sep 1, 2017

I've had this discussion with myself a few times.

I grew up on camel case - and have always used it - I've never worked for a company that insisted on underscores in their style guides. It's also what I teach in the classroom, and recommend my kids use. It's also more close to what most of the other languages in my classroom look like (Java, VB, C#, SmallBasic,, C++) - so makes for easier transitions...

When I started teaching python a couple of years ago, I had no idea there was such a thing as a PEP, and I had no idea python recommended using underscores. I hadn't come across them in any of the basic python commands or libraries, in the turtle package (apart from begin_fill and end_fill - which always looked so out of place), or in the basic tkinter commands. So I stuck with what I knew.

As I've never used underscores, I find them hard to read and annoying, and hard to teach - it's an unfamiliar key for young kids, and an extra key press when they're typing...

So, I've convinced myself camel case is better - obviously through my rose tinted glasses...

Now that I'm enjoying the micro:bit, I don't really find the underscores an improvement - they make it hard to fit code on the screen as lines get so long... But, it would be nice to have some consistency across the things I do with Python.... And, if appJar becomes useful to people, I suppose it would be nice to comply with the python style guides...

As to the function names - again they're like that from a teaching point of view - I wanted consistency along with self-explanatory code. A lot of them have been driven straight from the kids' expectations; once they get the basics, they start writing their GUIs and just expect commands to be there, following the same syntax - so I add them in...

The same is true of parameters. The kids struggle at first with parameters, with named parameters being another step up - so I have lots of set functions, instead of lots of named parameters.

Ultimately - the library has been designed for 11 year old kids. I don't want to lose the ease of use for them, but at the same time I would like it to become more friendly for developers...

I could have duplicate function names, to offer both styles, but that seems like overkill, especially with the size of the file already (again - it's all in one file to make it as easy as possible to get going).

So I'm unsure what to do...

It would be a massive task to get it changed, and with me back in school next week - there's not going to be much time for changes!

@mpmc

This comment has been minimized.

Contributor

mpmc commented Sep 1, 2017

I grew up on camel case - and have always used it
As did I, I got used to seeing it & even using it when I had to, but to me it felt wrong. I started with VB, but soon moved on to PHP, which I've been writing since I was 12!

I'm really glad to see programming languages being taught now, when I was at (high) school, we were just taught about word processing, Microsoft Word, Excel, Powerpoint etc. I cringe every time I think of, or see "mailmerge". This is at a time when we had mixed Windows 95, 98, Windows 2000, XP AND 3.1 with networking in different rooms.

IIRC I was once asked to "Design a website in powerpoint", which to me made absolutely no sense! It was extremely boring & a real pain for me (you'll find out why as you read on). I did it using PHP instead. Of course I failed, the teacher had no idea what PHP was, or simply refused to acknowledge it existed.

As I've never used underscores, I find them hard to read and annoying, and hard to teach - it's an unfamiliar key for young kids, and an extra key press when they're typing...

I can see why you prefer camel case, but for me capitals are a pain in the backside, mostly because I only use one hand and have to do a lot of moving and stretching to one side of the keyboard to reach modifiers & the button at the same time.

they make it hard to fit code on the screen as lines get so long...

Yes, this can be true in many cases, I try and keep my code =< 80 lines for this reason. But with Python long lines aren't so much of a problem, for example.

        self.setSticky('nesw')
        self.addLabel('FOOOOOOOOBAAAAARRRRRRRRRR', 
                      self.lang('WOOOOOO_HOOOO'), 0, 0)

But this is again down to personal preference :).

So I'm unsure what to do...

Continue as is! I and I'm sure many others will get used to it!


This is what my current code looks like using appJar. https://gist.github.com/mpmc/24f055b88be547c62a6d35fe51e63cf5

@keithterrill

This comment has been minimized.

keithterrill commented Sep 1, 2017

My two cents regarding naming scheme.

  1. Sir, you are a teacher, instructor. I believe that the simplicity of your code and the project is reflected in this fact. As you go forward and ponder your statement, "So I'm unsure what to do..." I recommend you keep KISS (Keep It Simple Silly) in mind.

  2. The PEP-8 (https://www.python.org/dev/peps/pep-0008/) reminds us, "do not break backwards compatibility just to comply with this PEP!" It also tells us, "A style guide is about consistency. Consistency with this style guide is important. Consistency within a project is more important. Consistency within one module or function is the most important."

  3. mpmc may find that CamelCase is harder to read, but there are many of us who do not find that to be the case. I too am old school. I began programming in 1976.

  4. Yes, addLabelNumericEntry could be replaced with "entry" or "numeric_entry", but at a loss of information. addLabelNumericEntry tells me 4 things: 1) we are adding the widget not reopening, 2) it contains a label in the grid cell, 3) it is a numeric not a string entry, and 4) it is an Entry field. It appears that the only way to change this while keeping the information in the name would be to use add_label_numeric_entry, which is cumbersome and takes up more real estate.

  5. Your current style has produced a very readable and self documenting project.

  6. It is my humble opinion that you do not need to change the style, nor should you. As you point out, "It would be a massive task to get it changed..."

  7. By the way: very good work, job done well.

@jarvisteach jarvisteach added the question label Sep 2, 2017

@jarvisteach

This comment has been minimized.

Owner

jarvisteach commented Sep 5, 2017

Thank you both for the support - I have some longer term aims, in terms of refactoring certain parts (widget management being the big one) - now looking more like V2.0 stage - and I will probably look into this sort of stuff then...

@jarvisteach jarvisteach closed this Sep 5, 2017

@jarvisteach

This comment has been minimized.

Owner

jarvisteach commented Nov 5, 2017

So, I've been giving this some more thought.

I think there are now too many functions for doing very similar things. Using entry (the busiest of the widgets) as an example:

app.addEntry()
app.addLabelEntry()
app.addValidationEntry()
app.addNumericEntry()
app.addDirectoryEntry()
app.addSecretEntry()
app.addFileEntry()
app.addAutoEntry()

Combining this with all the SET & GET functions I think is starting to make appJar quite difficult to get to grips with.

What I'm thinking is providing a single function per widget:

app.entry(title, value=None)

This will then serve all three functions - ADD/GET/SET.
It can use a bit of logic to determine what the user wants:

IF title exists:
    IF value is None: GET
    ELSE: SET
ELSE:
    ADD

This will provide a nice consistent approach, and achieves almost as much as we have now. The only thing lacking is the warning of duplicate widgets. If you are trying to ADD a widget, but use a duplicate name, you will mistakenly SET the original widget. I think this is an acceptable trade-off.

It can then wrap up all the different options using optional **kwargs:

secret=False # make it secret
type in ["validation", "directory", "numeric", "file", "auto"] # choose the type
default=None # set a default
label=False # show a label
limit=None # set a length limit
focus=False # give it the focus
case in ["upper", "lower"] # make all text upper/lower case

I think this is very achievable, and for now, can be added as a wrapper around most of the other functions, and run in parallel with what we already have.


Stage two is to look at all the set functions that get magically created. Again, I think these are getting unmanageable. I think a configEntry() function could be provided using **kwargs to set all the properties. I think this will be done under a separate change request.


Not sure what the longterm plan should be - will definitely implement these and run them in parallel, with documentation. Perhaps medium term, if they prove popular, will look to deprecate the old functions and remove previous documentation. Will hold out on that for now though...

@jarvisteach jarvisteach reopened this Nov 5, 2017

@jarvisteach jarvisteach added new feature and removed question labels Nov 5, 2017

@jarvisteach jarvisteach added this to the 1.0 milestone Nov 5, 2017

@mpmc

This comment has been minimized.

Contributor

mpmc commented Nov 5, 2017

This is a nice idea, and the fact it's going to be backwards compatible is even better, as I've gotten used to the method names already in place!

This will provide a nice consistent approach, and achieves almost as much as we have now. The only thing lacking is the warning of duplicate widgets. If you are trying to ADD a widget, but use a duplicate name, you will mistakenly SET the original widget. I think this is an acceptable trade-off.

I think this is what some people expect anyway.

It can then wrap up all the different options using optional **kwargs:

This is pretty much what I do with kwargs.

def foo(param, **kwargs):
    print(param)
    optional = kwargs.get('weird') or False
    if optional is True:
        return print('I\'m weird!')
    print('I\'m not weird!')

jarvisteach added a commit that referenced this issue Nov 5, 2017

Functions to support #235
All new functions for ADD/GET/SET widgets.

Also, added:
* getImage()
* getButton()
* getLink()
* setLink()
* getMessage()

jarvisteach added a commit that referenced this issue Nov 5, 2017

Changed label #235
Label not takes a type.

Updated docs

jarvisteach added a commit that referenced this issue Nov 6, 2017

jarvisteach added a commit that referenced this issue Nov 6, 2017

jarvisteach added a commit that referenced this issue Nov 7, 2017

jarvisteach added a commit that referenced this issue Nov 7, 2017

AutoEntry fixes
Fixed AutoEntries in subWindows - #290
Can now append to autoEntry - #289
Changes to label() - can now add empty label - #235
@jarvisteach

This comment has been minimized.

Owner

jarvisteach commented Nov 7, 2017

All basic widgets now implemented - couple of issues to resolve.

Most of the widget specific set functions now implemented as parameters to the ADD function:
app.entry("name", default="Enter username", focus=True, limit=12)

Don't support labelled widgets yet, would like a boolean label flag to show a label. But want to refactor labelelled widget creation, rather than have conditional constructor calls.

Documentation started here (needs a lot of work): https://github.com/jarvisteach/appJar/blob/next_release/docs/mkdocs/docs/simpleAppJar.md

jarvisteach added a commit that referenced this issue Nov 7, 2017

AutoComplete functions & updated docs
New functions for AutoComplete #289 - need wrappers. Also reduced width
of list, so a little narrower than associated entry.

Updated docs #235

jarvisteach added a commit that referenced this issue Nov 7, 2017

jarvisteach added a commit that referenced this issue Nov 7, 2017

jarvisteach added a commit that referenced this issue Nov 10, 2017

Entry refactor
Start of refactoring for #235.

All `.add*Entry()` functions now call `.__entryMaker()`

That in turn calls the relevant `.build*Entry()` function, as well as
making a labelFrame if required.

The `.entry()` function now also calls `.__entryMaker()`

Also, resolved some entry issues #292:
* secretEntries now support defaults
* file/directory entries better support setting defaults

Updated tests & docs

jarvisteach added a commit that referenced this issue Nov 10, 2017

GoogleMapMarker update
Additional features for mapMarkers #174 & #293

Can now specify a size, colour, label, and option to replace the
previous pin.

This required storing a dictionary of marker details, and updates to
the generate URL function.

Also, added change options for listBoxes & optionBoxes #235

Updated docs
@mpmc

This comment has been minimized.

Contributor

mpmc commented Nov 11, 2017

@jarvisteach

I'm currently rewriting my hs602/controller class (for a 5th time 😦 ) after discovering the @property decorator, and wondered if you used it in appJar, took a look & saw that you don't so decided to mention it here. I'm finding it extremely useful, hope you do too 👍

@jarvisteach

This comment has been minimized.

Owner

jarvisteach commented Nov 12, 2017

Yeah - I've had a think about this. And originally wanted to use it, but couldn't see how it would fit...

The point of the property decorator is to allow you to access getter/setter functions on attributes by simply referencing their name.

But for that to work, setting should require a single value and getting should require None.

So, for example:

gui.label = "Hello World"
print ( gui.label )

That model doesn't really work in appJar, as both of the above functions would also require the widget's name as a parameter.

So, for setters, we could solve it by passing a tuple of values and unpacking them:

# original style
app.addLabel("a", "Hello")
app.setLabel("a", "new hello")
print ( app.getLabel("a" )

# new style being introduced
app.label("b", "Hello")
app.label("a", "new hello")
print ( app.label("a" )

# as a property setter
app.label = ("c", "hello")
app.label = ("c", "hello")
print ( app.label("c") )
# or
app.label = "c", "hello"
app.label = "c", "hello"
print ( app.label("c") )

I think each one is less self-documenting than the previous, and less intuitive (remembering the primary target is school kids).

I could have skipped the new style being introduced and wrapped the getLabel & addLabel up as properties, but I really don't like the syntax necessary to support the extra parameter...

@mpmc

This comment has been minimized.

Contributor

mpmc commented Nov 12, 2017

I see, I honestly didn't see any use for it either, but I stuck with it..

This is what my code looks like now using properties (still WIP) - I'm just glad I could reuse most of the existing code & tweak things as needed.

@mpmc

This comment has been minimized.

Contributor

mpmc commented Nov 18, 2017

Trying the new renamed widgets, getting an error with list.

Exception in Tkinter callback
Traceback (most recent call last):
  File "/usr/lib/python3.6/tkinter/__init__.py", line 1702, in __call__
    return self.func(*args)
  File "/usr/lib/python3.6/tkinter/__init__.py", line 748, in callit
    func(*args)
  File "/home/mark/.local/lib/python3.6/site-packages/appJar/appjar.py", line 1903, in __processEventQueue
    func(*args, **kwargs)
  File "gui2.py", line 278, in result
    self.list('addr', devices, row=1, column=0)
  File "/home/mark/.local/lib/python3.6/site-packages/appJar/appjar.py", line 6208, in list
    return self.listBox(title, value, *args, **kwargs)
  File "/home/mark/.local/lib/python3.6/site-packages/appJar/appjar.py", line 6227, in listBox
    if change is not None: self.setListBoxChangeFunction(title, change)
  File "<string>", line 1, in setListBoxChangeFunction
  File "/home/mark/.local/lib/python3.6/site-packages/appJar/appjar.py", line 2529, in configureWidget
    self.__bindEvent(kind, name, item, value, option, key)
  File "/home/mark/.local/lib/python3.6/site-packages/appJar/appjar.py", line 2695, in __bindEvent
    cmd = self.MAKE_FUNC(function, name, True)
  File "/home/mark/.local/lib/python3.6/site-packages/appJar/appjar.py", line 3069, in MAKE_FUNC
    raise Exception("Invalid function: " + str(funcName))
Exception: Invalid function: False

I'm also not sure on calling it list, it could be confused with pythons own list()?

Example being

list(app.list(foo, bar))

jarvisteach added a commit that referenced this issue Nov 18, 2017

@jarvisteach

This comment has been minimized.

Owner

jarvisteach commented Nov 18, 2017

Fixed the error - was defaulting to False instead of None.

I agree on the naming - there are a couple in there I'm unsure of. I'm trying to keep each widget to a single word, any suggestions? There are also some param names that will probably change (type)...

jarvisteach added a commit that referenced this issue Dec 17, 2017

New pos parameter #235
Can now pass RCS as pos tuple

jarvisteach added a commit that referenced this issue Dec 17, 2017

@mpmc

This comment has been minimized.

Contributor

mpmc commented Dec 19, 2017

I've just tried to pass align to a label but it's not accepted :(.

e.g.

label('foo', 'bar', align='center')
@mpmc

This comment has been minimized.

Contributor

mpmc commented Dec 19, 2017

Latest code breaks :(

Exception in Tkinter callback
Traceback (most recent call last):
  File "/usr/lib/python3.6/tkinter/__init__.py", line 1702, in __call__
    return self.func(*args)
  File "/usr/lib/python3.6/tkinter/__init__.py", line 748, in callit
    func(*args)
  File "/home/mark/.local/lib/python3.6/site-packages/appJar/appjar.py", line 1945, in __processEventQueue
    func(*args, **kwargs)
  File "gui3.py", line 165, in callback
    self.button(_('\u26B2 Scan again'), self.discover, 2, 0, 1)
  File "/home/mark/.local/lib/python3.6/site-packages/appJar/appjar.py", line 6683, in button
    else: button = self._buttonMaker(title, value, "button", extra=None, *args, **kwargs)
TypeError: _buttonMaker() got multiple values for argument 'extra'
@jarvisteach

This comment has been minimized.

Owner

jarvisteach commented Dec 20, 2017

@mpmc I don't think align is a valid argument, try setting anchor="e" - there's better error handling in the newer commits.

The second error is an interesting one - it has nothing to do with the extra parameter, and is in fact because I've named some parameters, but then *args has some unnamed parameters - I'll fix that...

jarvisteach added a commit that referenced this issue Dec 20, 2017

@mpmc

This comment has been minimized.

Contributor

mpmc commented Dec 20, 2017

Ok thanks
I'll wait for you to release 0.90 as I have some new ideas for my GUI 😄

jarvisteach added a commit that referenced this issue Jan 6, 2018

New properties #340
Also, updated docs & added listBoxMaker #235

jarvisteach added a commit that referenced this issue Jan 8, 2018

testing #339
Added option so `.label()` can receive a single parameter, and will use
it as its text & title #235.
@jarvisteach

This comment has been minimized.

Owner

jarvisteach commented Jan 14, 2018

Tracking normalisation of simpleGUI.

Should all include:

  • kwargs = self._parsePos(kwargs.pop("pos", []), kwargs)
  • self._configWidget(title, label, widgKind, **kwargs)

And new sequence:

# POP ANY BESPOKE SETTERS
try: self.widgetManager.verify(widgKind, title)
except: # widget exists
    if value is not None:
        # SET WIDGET
    # GET WIDGET
else:
    # CREATE & GET WIDGET
# CALL ANY BESPOKE SETTERS
# CONFIG WIDGET
# RETURN WIDGET

  • label
  • message
  • canvas
  • entry
  • text / textArea
  • button
  • link
  • check / checkBox
  • radio / radioButton
  • option / combo / optionBox
  • spin / spinBox
  • list / listBox
  • slider / scale
  • meter
  • grip
  • separator
  • image
  • properties
  • date / datePicker
  • microbit
  • map
  • pie
  • tree
  • grid
  • plot

  • popUp

  • statusBar
  • toolBar
  • menuBar
@jarvisteach

This comment has been minimized.

Owner

jarvisteach commented Jan 14, 2018

Must also ensure callFunction is propagated to setters.

jarvisteach added a commit that referenced this issue Jan 14, 2018

Fix for #347
Investigate #346
fix for #347 - `.removeAllWidgets()` no longer disables rightClickMenu
& new function `.disableMenuEdit()`
More work on #235

jarvisteach added a commit that referenced this issue Jan 16, 2018

Improvements for #235
Updated `._configureWidget()` to find the widget itself, saves passing
it from the calling function.

Changes to radio & check - to better support selecting.

jarvisteach added a commit that referenced this issue Jan 16, 2018

jarvisteach added a commit that referenced this issue Jan 18, 2018

jarvisteach added a commit that referenced this issue Jan 20, 2018

jarvisteach added a commit that referenced this issue Jan 21, 2018

updates to ttk #189 - theme now supported in contructor & as a proper…
…ty.\nmethod naming #235 - list renamed to listbox, _parsePos now accepts sticky & stretch

jarvisteach added a commit that referenced this issue Feb 13, 2018

Playing with issue #328
Pop-up for configuring GUI settings.

Fixed a couple of issues (#235, #340):
* getFonts was returning None, now works & has property
* subWindow can take kwargs
* checkBox simple setter wasn’t working
* optionBox simple setter now takes a selected param
* tip param introduced
* buttons simple setter introduced

jarvisteach added a commit that referenced this issue Feb 13, 2018

Updated layout settings #301
RCS can now include `previous` and `next`
Moved parsePos to add section of simpleNamers
DatePicker now takes “today” as a param. #175
#235 changes:
Updated buttons to handle 2D list
Added kwargs to tab & panedFrame
Updated showcase to use new features

jarvisteach added a commit that referenced this issue Feb 15, 2018

More widgets #235
Added:
* pie
* tree
* microbit
* plot
* map

Updated:
* separator
* meter
* date picker
* canvas
* checkBox

jarvisteach added a commit that referenced this issue Mar 1, 2018

simpleAppJar statusbar #235
functions to add simple status

jarvisteach added a commit that referenced this issue Mar 2, 2018

simple statusbar updates #235
Updated docs & testing for simple status bar

Also, new function to hide all subWindows #208

jarvisteach added a commit that referenced this issue Mar 2, 2018

jarvisteach added a commit that referenced this issue Mar 2, 2018

jarvisteach added a commit that referenced this issue Mar 2, 2018

Updated docs #395
Also fixed issue with status bar & testing #235

jarvisteach added a commit that referenced this issue Apr 15, 2018

@jarvisteach

This comment has been minimized.

Owner

jarvisteach commented Apr 15, 2018

Going to close this thread - everything is implemented now, except the menubar. Will include that in the tidy menubar function.

Now the docs need resolving, again will cover that in a separate issue.

@jarvisteach jarvisteach referenced this issue Apr 15, 2018

Open

Refactor menus #305

6 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment