Skip to content

Hacking

Ales Kozumplik edited this page · 56 revisions

Overall Direction

There has to be a valid use-case (preferably external) for every new feature. Adding code that doesn't get called in production, or only very rarely, makes the codebase difficult to get into and to maintain.

Do not postpone refactoring ugly/tricky/hackish code. Otherwise everybody who has to touch it until it is brought up to par pays a price for it. This sums up and compounds over time.

Do not introduce new configuration options if at all possible. The users don't care, don't want to read the manual, don't want to think too much. They want everything to work instantly, like a magic.

With all other things being equal, more maintainable is better than less maintainable. This often implies shorter code but sometimes it does not. Spending a few hours on fine tuning that functional oneliner that takes somebody else twenty minutes just to decode is not more maintainable.

In the end we just want to deliver a solid, fully-fledged package manager and move on with our lives. Working on a project forever just for the sake of it has no meaning.

Coding Style

Style-wise and design-wise we prefer maintainability and readability over simplicity/succinctness of the patch.

Adhere to PEP 008 <http://www.python.org/dev/peps/pep-0008/> and PEP 257 <http://www.python.org/dev/peps/pep-0257/> as much as reasonable.

Test classes should be called TheThingTest where TheThing is the thing being tested.

Keep the lines around 80 characters max. Splitting is strongly recommended if the line has 82 or more characters.

Trailing whitespace is forbidden, so is commented out debug code or XXX and TODO comments. Either it needs to be fixed, then fix it before making a pull request, or such comments are just noise.

Python module imports should be in an alphabetical order, near the top of the file. Avoid from x import y imports in cases where y is referenced less than five times in the source file (readability). The section of from x import y imports, if any, precedes the section of import x imports in the source file, if any. Do not import more than one module per line. Do not introduce any star imports.

Alphabetical ordering applies in general: not only it makes it easier for one to decide where to put a new constant in a list of constants, it also causes two concurrent patches to put it in the same place so the eventual merge doesn't conflict (in theory anyway).

When both single quote (') and double quotes (") are practical when denoting a string literal, use single quote ('). This convention is not observed for docstrings where we use """ just like most Python projects.

Try to avoid naming things by their type. E.g. if it's a topical branch in git don't call it my-dnf-project-branch, as both 'dnf' and 'branch' are redundant in this context. Similarly, if it's a class doing some "magic" don't call it MagicClass, simply Magic is fine.

In similar vein, do not prefix names with the obvious. Names like yumbase or YumBase are not practical because they are longer, inconsistent and even wrong if the project changes. Python has reasonable namespacing facilities by itself.

In classes, put classmethods, static methods and properties first. The instance methods go after that. If in doubt order things alphabetically.

The current DNF code runs under both Python 2 and Python 3, any new code must obey this limitation.

Tags

Use the following in-code:

:deprecated <dnf version># in docstring, a deprecated code path. add the version when the
deprecation was first introduced.
:api # within a comment or a docstring, the item (class, method) is a part of the public API
:binformat # value is used for file output (i.e. should not change between versions)

Use the following in the title line of commit messages:

apichange: # changes the public API
refactor: # refactoring job
move: # involves moved files
remove: # removal of no longer used parts/files
build: # new build or version
tests: # only relevant for running the unit tests, doesn't change live code
doc: # only relevant for the documentation, doesn't change live code
RhBug:<number> # red hat bugzilla number this commit fixes
Related: RhBug:<number> # related to this red hat bugzilla number, partial fix.

Logging

DNF uses simplified logging model from Yum. There are fewer logging levels, those from the standard logging module and three custom ones: SUBDEBUG, DDEBUG and SUPERCRITICAL. SUPERCRITICAL is not to be used, it's meaning is only to define the level where the logging Handlers shouldn't let through anything. The table below summarizes the usage of different log levels:

level terminal with -v translate
CRITICAL stderr stderr yes
ERROR stderr stderr yes
WARNING stderr stderr yes
INFO stdout stdout yes
DEBUG n/a stdout no
DDEBUG n/a with -d 10 no
SUBDEBUG n/a n/a no

In the future it should be possible to introduce a new gettext keyword that creates object with both the translation and the original string. This way we could write things like:

log_and_print(logging.INFO, _x("Phony Beatlemania has bitten the dust."))

and have DNF output the translation to the stdout and the original log message to the logfiles.

It is a good practice not to explicitly perform any conversions or % substitutions while calling the logging functions. E.g. instead of:

logger.debug('that thing %s did that %d times' % (str(obj1), int(obj2)))

just do:

logger.debug('that thing %s did that %d times', obj1, obj2)

The idea is that if the logger decides the message has a level too low to be logged the operations don't need to happen at all. In DNF everything is logged into the logfile though so this doesn't exactly apply but Pylint will report this still.

If you realize that the output is something the user should see and it is not interesting from the logging perspective, just use print() (the function, not the statement, see the note below print()'s documentation). Strings printed in this way should always be translated.

The base.conf.verbose attribute is True whenever DNF was run in the verbose mode and so it is a natural way to decide how much to print().

Unit Testing

The rules for unit tests are quite loose. Generally, try to make the unit tests run as quickly as possible, without redundant tests. Do not introduce new testing repositories if at all possible. Generally, mock out things that depend on or change the state of the system the test is running on (filesystem operations, rpmdb modification). If you have to write files out onto the system, make sure the test cleans up after itself. Also, don't make the test cases a mocking orgy: if the stack below can handle particular operation quickly and without side-effects to the host system then consider using it instead of a mock.

When creating new mocks, please respect following terminology: stubs are easily-created objects pretending to be another objects, mocks are stubs recording calls on themselves and fakes are general term for all these objects (taken from the last paragraph in wikipedia.org/wiki/Mock_object#Mocks.2C_fakes_and_stubs).

Review

Make sure your commits are rebased against the top (or near top) of the branch you want to merge into. When the reviewer asks you to update some of your patches, amend the updates to the respective patches instead of creating new patches fixing your previous patches that are not yet in the upstream branch anyway.

Also, common sense applies: make the commits fairly granular, if your change requires a change of something else (e.g. a fix or extension to an existing interface), put it in a separate commit. The commit message should be brief yet descriptive, mentioning the ticket it is related to/fixing. Try not to go over 80 characters line width.

Desirable Improvements

The following are changes improving the source quality that we would like to see the most and won't say no to. In no particular order and in different stages of completeness:

  • remove dependency of dnf.cli.Output on the whole dnf.cli.Base
  • put tests/test*.py to the proper subdirectories.
  • break out concrete commands from dnf.cli.commands to their own modules.
  • no unused imports
  • no Python source files over 1k LOC.
  • no Python methods over 40 LOC, unless they are purely decision-making (i.e. long list of if-elifs)
  • built-in commands should properly use cli.demands instead of class attributes.
  • no __del__ methods. If cleanup is vital, use a context manager.
  • refactoring and moving out everything in dnf.yum
  • fixing problems reported by Pylint
Something went wrong with that request. Please try again.