Skip to content

[dev] Coding Style

Jump to bottom
Utumno edited this page Oct 19, 2024 · 92 revisions

This is another WIP but once we agree it will be set in stone - meaning hooks rejecting commits that do not conform - and probably remotely deleting your saved games. You've been warned.

  • Follow PEP 8, readability counts.
    • Wrap your code: new code should conform to the PEP8 79 characters rule - discussion.
    • Whitespace: do not follow PEP8 two newlines between classes guideline - one newline (existing convention + we really can't make the files even bigger). Do follow PEP8 on the rest on newlines (including avoiding them as hell). If you miss blank lines, time to consider helper methods. All files should end in one newline (see also). No trailing spaces.
  • Don't copy/paste code. Extract methods.
  • Do comment your code. Refer back to PEP 8 on what to comment.
  • Spell check. We generally use American English, i.e. US spelling (see also [dev] GUI Design Guidelines, translatable strings must be American English).
  • Prepend unused variables with an underscore (see here).
  • Never name something the same as a builtin.
  • _ is reserved for the translation function - do not use it for iteration variables, that will blow up!
  • All Python files shebangs should be universally portable. Use this: #!/usr/bin/env python.
  • Prefer single quotes for strings, except when the string contains single quotes.
  • Inner functions must start with an underscore.
  • Type hints: use them where it doesn't make the code less readable. See the wiki page for more information, policy on them is not set yet.
  • Comparisons to enum values should be done via is, not ==.

Note on globals() and co: forbidden. Used in places in the code (most notably in the patcher dialog and in setting the game) but this is not to be imitated and actually those uses must be revisited.

Variable naming: do not name your local variable (or even worse, a class attribute) data, items or, for heaven's sake, list. If in doubt do a search - if your newly named variable has some 100 occurrences use a more special name - except if this is a standard convention in existing code. Always choose a descriptive name for your variable/attribute (there are were a couple of thousand occurrences of the word data and half a thousand items in the code) and never a python builtin/method. If possible, try to come up with completely unique names - but never sacrifice readability just for that!

Forbidden variable/attributes names

Builtins - like file, dir, type, format, buffer - and things like (expect the list to grow):

data # a bane in 305, mostly fixed now
item(s) # similar to data, in the Tanks/List code
selected # see above, Links code + mods (now active and co)
refresh # bosh.DataStore#refresh, ~100 occurrences each one carefully thought about
data_store # UIList attribute - only in balt/basher, never a local variable
           # one of the marks of bosh/basher boundary
attr(s)
mtime
size # nasty one - rename local variables to more descriptive names
size_ # reserved for loadData
path
name # please no, no NOOOO
choice(s)
text
pack
unpack
_text # ONLY used for ItemLink's private attribute
help
_help # ONLY used for ItemLink's private attribute
archive(s)
sort(ed) # builtin status, hey
setting(s)
master(s)
abs_path # bolt.AFile#abs_path, thanks
merged, imported (with or without leading underscore(s))
fullpath
factory
group # an Installer attribute (yak) - and there is re.group...
flags # everywhere, thanks - and there is re.compile(..., flags=...)
exe # at least in core code, in scripts/ should be ok
espm # anything involving this, currently everywhere (300+); outdated since ESLs, use plugin instead
types # builtin module plus used in all kind of places
aliases # used in various roles in patcher and gui code
tip # says nothing, was everywhere in patcher / gui code
fid # used as an attribute to various records classes - rename local variables
key # especially bad as used in patchers code as attribute, builtin status sort(key=...).
    # If you're looking for `for key, value in ...`, use `for k, v in ...` instead
ini, bsa, ess, esp... # basically any file extensions, we want those reserved for, well,
                      # file extensions
Ini, Bsa, Ess, Esp... # GameInfo subclasses *only*!
option, grp # fomod & gui_fomod only
rec_sig # used for the MreRecord header signature class variable
parent # gui only - still spread throughout the code, #190
temp # bolt.Path.temp is gone, but this is still way too short and generic
Bain # GameInfo subclass
_j # os.path.join shorthand
mod_names # a Cosaves attribute
installer # a key in modInfos table
table # only use for FileInfos table attr
fmt # records refactoring - hunt down along with attr
next # builtin, plus breaks 2to3 fixers
default # records but also parameter in library functions (and some of our own - avoid)
full # we're full of it
level # records vs _saves, rename there
rfid_? # use only in iter_present_records loops
debug # reserved for eventual better logging
inst # reserved for Installer instances
_?rid # used in iterations of getActiveRecords
match # builtin status since 3.10 (pattern matching) - plus re.match is a thing
minf # reserved for variables that can be a Master or Mod Info
game_handle # inject the GameInfo singleton to LoGame, PluginFlag etc
_game_handle # private attr of said hierarchies

PEP8 naming

Currently, Bash uses mixed style - for the greater part the APIs introduced in 306 and the code we write to these APIs use pep8_style while older code traditionalBashStyle. We did it this way cause initially we just mimicked the coding style of surrounding code but as the APIs matured we started using new_style. This gives a historical perspective which, trust me, helps in groking the code. Camel case regions are code that is not (heavily) rewritten and should probably be revisited. Another issue is wx - MethodsAreLikeThis - so one part of Bash should/can not be PEP8'd anyway. Bottom line:

  • new code you contribute should be PEP8 as far as naming is concerned. If you go ahead and edit old code you can after the essential changes are reviewed PEP8 the API you rewrote, meaning the method/functions/classes names - PEP8'ing local variables will just add noise.
  • code you edit you can PEP8 as you go of course, down to variable names - it's "new code"
  • if in doubt follow dev commits and ask
  • once the refactoring freezes we might consider PEP8'ing - not trivial due to the around 8k (!) PEP8 warnings currently emitted, but most importantly due to wx. The end goal (in the far future) is to be able to judge by the CodeStyle of methods alone if the code at hand is directly or indirectly wx bound or pure Python.

2018-11-28: Holy cow - what I intended to do is discussed by Joel Spolsky in Making Wrong Code Look Wrong:

This is the real art: making robust code by literally inventing conventions that make errors stand out on the screen.

Underscores

Use them! It is very useful in 100k lines of code to specify what class method/attributes are supposed to be used by clients and what is an implementation detail. Plus sorts well in the debugger. Unfortunately Bash was not written with encapsulation in mind - new code however is. In general:

  • Leading underscores in methods and attributes: mark internal class API, wx plumbing etc - good code health. Keep in mind that internal API is in the making.
  • Trailing underscores: signal shadowing - bad code health.
  • Leading underscores in method parameters: monkey patches, WIP APIs etc - very bad code health. Except if the method parameter is indeed only used inside the class, which may be good.
  • Double leading underscores in method parameters: performance hacks. Accessing these is faster than accessing the globals they point to (true as of Python 3.10).

Exceptions

Do not base your control flow on exceptions. Google c++ style guide forbids exceptions - in python we can't quite, but certainly avoid them in your methods. Exception-throwing methods are difficult to reason about - unfortunately, unlike Java's, Python's exceptions are not required to be caught or declared in the method signature leading to even more confusion - as a lot of methods (including some std ones) throw undocumented exceptions (things are fixed in py3).

General

  • Patches with a negative line count will be preferred. Less code more features is a great motto.
  • Code wrapping at 79 chars (and line count) is a measurable code metric - a space after a comma is not. Prefer omitting the space than splitting the line.
  • One line of code equals one sentence in English

PY3

  • fstrings: we are currently converting debug/internal strings to fstrings - yey. Still toying but seems best:
    • to avoid unnecessary specifiers such as f'{len([1,2]):d}' -> f'{len([1,2])}' - less visual clutter
    • currently, I keep repr format specifier as in f'{obj!r} - the reason is that most of the time we use repr in those debug strings something is amiss (also pycharm automated fstringifier uses this format, we can always edit in bulk or while we revisit)
Clone this wiki locally