Join GitHub today
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 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.
Style-wise and design-wise we prefer maintainability and readability over simplicity/succinctness of the patch.
Test classes should be called
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
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
Magic is fine.
In similar vein, do not prefix names with the obvious. Names like
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.
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.
DNF uses simplified logging model from Yum. There are fewer logging levels,
those from the standard
logging module and three custom ones:
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:
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)))
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.
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
It is good practice to provide additional information for translators directly above string like:
But if the information is not directly above the string, the information will be not passed to translators. Like in the bad example here:
self.__run_message_dialog( # TRANSLATORS: here is a comment _("IBUS_ENGINE_NAME environment variable is not set."), Gtk.MessageType.WARNING)
But take care that xgettext is called with the -c option! From the man-page of xgettext:
# TRANSLATORS: here is a comment self.__run_message_dialog( _("IBUS_ENGINE_NAME environment variable is not set."), Gtk.MessageType.WARNING)
To add the -c option, one may need to add that option to a Makevars file in the po directory, as it is done for example in ibus-typing-booster here:
-cTAG, --add-comments=TAG place comment blocks starting with TAG and preceding keyword lines in output file -c, --add-comments place all comment blocks preceding keyword lines in output file
In case of ibus-typing-booster, xgettext is called as follows when executing "make dist":
/usr/bin/xgettext --default-domain=ibus-typing-booster --directory=.. \ --add-comments=TRANSLATORS: -c --keyword=_ --keyword=N_ \ --files-from=./POTFILES.in \ --copyright-holder='Anish Patil <firstname.lastname@example.org>' \ --msgid-bugs-address="$msgid_bugs_address"
When writing a code that depends on another piece of software, check whether the dependency is already present in the spec file. If not or if the version specified there is too low, change it immediately.
Similarly when submitting a backwards incompatible API change, set the Version tag to
(MAJOR+1).0.0 and the Release tag to
0.1.alpha plus the macros immediately (if not done already). At the same time, it's also needed to mark the previous interface as deprecated and bump (if not bumped already) the minor version of DNF in the branch of the latest stable release.
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).
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.
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.Outputon the whole
tests/test*.pyto the proper subdirectories.
- break out concrete commands from
dnf.cli.commandsto 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.demandsinstead of class attributes.
__del__methods. If cleanup is vital, use a context manager.
- refactoring and moving out everything in
- fixing problems reported by Pylint