diff options
Diffstat (limited to 'docs/contributing/coding_guidelines.rst')
| -rw-r--r-- | docs/contributing/coding_guidelines.rst | 164 |
1 files changed, 109 insertions, 55 deletions
diff --git a/docs/contributing/coding_guidelines.rst b/docs/contributing/coding_guidelines.rst index 9c996518..634c3e61 100644 --- a/docs/contributing/coding_guidelines.rst +++ b/docs/contributing/coding_guidelines.rst @@ -3,21 +3,57 @@ Python Coding Guidelines ======================== -The switch to the Python programming language for new code is not merely a change -of the language, but a chance to rethink and improve the programming approach. +The switch to the Python programming language for new code is not merely a +change of the language, but a chance to rethink and improve the programming +approach. Let's face it: VyOS is full of spaghetti code where logic for reading the VyOS config, generating daemon configs, and restarting processes is all mixed up. -Python (or any other language, for that matter) does not provide automatic protection -from bad design, so we need to also devise design guidelines and follow them to -keep the system extensible and maintainable. +Python (or any other language, for that matter) does not provide automatic +protection from bad design, so we need to also devise design guidelines and +follow them to keep the system extensible and maintainable. + +But we are here to assist you and want to guide you through how you can become +a good VyOS contributor. The rules we have are not there to punish you - the +rules are in place to help us all. What does it mean? By having a consistent +coding style it becomes very easy for new contributors and also longtime +contributors to navigate through the sources and all the implied logic of +the spaghetti code. + +Please use the following template as good starting point when developing new +modules or even rewrite a whole bunch of code in the new style XML/Pyhon +interface. Configuration script structure and behaviour -------------------------------------------- +Your configuration script or operation mode script which is also written in +Python3 should have a line break on 80 characters. This seems to be a bit odd +nowadays but as some people also work remotly or programm using vi(m) this is +a fair good standard which I hope we can rely on. + +In addition this also helps when browsing the GitHub codebase on a mobile +device if you happen to be a crazy scientist. + .. code-block:: python + #!/usr/bin/env python3 + # + # Copyright (C) 2019 VyOS maintainers and contributors + # + # This program is free software; you can redistribute it and/or modify + # it under the terms of the GNU General Public License version 2 or later as + # published by the Free Software Foundation. + # + # This program is distributed in the hope that it will be useful, + # but WITHOUT ANY WARRANTY; without even the implied warranty of + # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + # GNU General Public License for more details. + # + # You should have received a copy of the GNU General Public License + # along with this program. If not, see <http://www.gnu.org/licenses/>. + import sys from vyos.config import Config @@ -50,36 +86,39 @@ Configuration script structure and behaviour print(e) sys.exit(1) -The **get_config()** function must convert the VyOS config to an abstract internal -representation. No other function is allowed to call ``vyos.config.Config`` object -methods directly. The rationale for it is that when config reads are mixed with -other logic, it's very hard to change the config syntax since you need to weed -out every occurrence of the old syntax. If syntax-specific code is confined to a -single function, the rest of the code can be left untouched as long as the -internal representation remains compatible. - -Another advantage is testability of the code. Mocking the entire config subsystem -is hard, while constructing an internal representation by hand is way simpler. - -The **verify()** function takes an internal representation of the config and checks -if it's valid, otherwise it must raise ``VyOSError`` with an error message that -describes the problem and possibly suggests how to fix it. It must not make any -changes to the system. The rationale for it is again testability and, in the -future when the config backend is ready and every script is rewritten in this -fashion, ability to execute commit dry run ("commit test" like in JunOS) and abort -commit before making any changes to the system if an error is found in any component. - -The **generate()** function generates config files for system components. - -The **apply()** function applies the generated configuration to the live system. -It should use non-disruptive reload whenever possible. It may execute disruptive -operations such as daemon process restart if a particular component does not -support non-disruptive reload, or when the expected service degradation is minimal -(for example, in case of auxiliary services such as LLDPd). In case of high impact -services such as VPN daemon and routing protocols, when non-disruptive reload is -supported for some but not all types of configuration changes, scripts authors -should make effort to determine if a configuration change can be done in a -non-disruptive way and only resort to disruptive restart if it cannot be avoided. +The ``get_config()`` function must convert the VyOS config to an abstract, +internal representation. No other function is allowed to call the ``vyos.config. +Config`` object method directly. The rationale for it is that when config reads +are mixed with other logic, it's very hard to change the config syntax since +you need to weed out every occurrence of the old syntax. If syntax-specific +code is confined to a single function, the rest of the code can be left +untouched as long as the internal representation remains compatible. + +Another advantage is testability of the code. Mocking the entire config +subsystem is hard, while constructing an internal representation by hand is +way simpler. + +The ``verify()`` function takes your internal representation of the config and +checks if it's valid, otherwise it must raise ``ConfigError`` with an error +message that describes the problem and possibly suggests how to fix it. It must +not make any changes to the system. The rationale for it is again testability +and, in the future when the config backend is ready and every script is +rewritten in this fashion, ability to execute commit dry run ("commit test" +like in JunOS) and abort commit before making any changes to the system if an +error is found in any component. + +The ``generate()`` function generates config files for system components. + +The ``apply()`` function applies the generated configuration to the live +system. It should use non-disruptive reload whenever possible. It may execute +disruptive operations such as daemon process restart if a particular component +does not support non-disruptive reload, or when the expected service degradation +is minimal (for example, in case of auxiliary services such as LLDPd). In case +of high impact services such as VPN daemon and routing protocols, when non- +disruptive reload is supported for some but not all types of configuration +changes, scripts authors should make effort to determine if a configuration +change can be done in a non-disruptive way and only resort to disruptive restart +if it cannot be avoided. Unless absolutely necessary, configuration scripts should not modify the active configuration of system components directly. Whenever at all possible, scripts @@ -89,34 +128,50 @@ one by one is particularly discouraged, for example, when configuring netfilter rules, saving them to a file and loading it with iptables-restore should always be preferred to executing iptables directly. -The **apply()** and **generate()** functions may ``raise ConfigError`` if, for +The ``apply()`` and ``generate()`` functions may ``raise ConfigError`` if, for example, the daemon failed to start with the updated config. It shouldn't be a -substitute for proper config checking in the **verify()** function. All reasonable -effort should be made to verify that generated configuration is valid and will -be accepted by the daemon, including, when necessary, cross-checks with other -VyOS configuration subtrees. - -Exceptions, including ``VyOSError`` (which is raised by ``vyos.config.Config`` on -improper config operations, such as trying to use ``list_nodes()`` on a non-tag -node) should not be silenced or caught and re-raised as config error. Sure this -will not look pretty on user's screen, but it will make way better bug reports, -and help users (and most VyOS users are IT professionals) do their own debugging -as well. +substitute for proper config checking in the ``verify()`` function. All +reasonable effort should be made to verify that generated configuration is +valid and will be accepted by the daemon, including, when necessary, cross- +checks with other VyOS configuration subtrees. + +Exceptions, including ``VyOSError`` (which is raised by ``vyos.config.Config`` +on improper config operations, such as trying to use ``list_nodes()`` on a +non-tag node) should not be silenced or caught and re-raised as config error. +Sure this will not look pretty on user's screen, but it will make way better +bug reports, and help users (and most VyOS users are IT professionals) do their +own debugging as well. + +For easy orientation we suggest you take a look on the ``ntp.py`` or +``interfaces-bonding.py`` (for tag nodes) implementation. Both files can be +found in the vyos-1x_ repository. + Coding guidelines ----------------- +Like any other project we have some small guidelines about our source code, too. +The rules we have are not there to punish you - the rules are in place to help +us all. By having a consistent coding style it becomes very easy for new +and also longtime contributors to navigate through the sources and all the +implied logic of any one source file.. + Language ******** -Python 3 **shall** be used. How long can we keep Python 2 alive anyway? - -No considerations for Python 2 compatibility **should** be taken. +Python 3 **shall** be used. How long can we keep Python 2 alive anyway? No +considerations for Python 2 compatibility **should** be taken at any time. Formatting ********** -Tabs **shall not** be used. Every indentation level should be 4 spaces. +* Python: Tabs **shall not** be used. Every indentation level should be 4 spaces +* XML: Tabs **shall not** be used. Every indentation level should be 2 spaces + +.. note: There are extensions to e.g. VIM (xmllint) which will help you to get + your indention levels correct. Add to following to your .vimrc file: + ``au FileType xml setlocal equalprg=xmllint\ --format\ --recover\ -\ + 2>/dev/null`` now you can call the linter using ``gg=G`` in command mode. Text generation *************** @@ -134,11 +189,10 @@ Code policy When modifying the source code, remember these rules of the legacy elimination campaign: - * No new features in Perl - * No old style command definitions - * No code incompatible with Python3 +* No new features in Perl +* No old style command definitions +* No code incompatible with Python3 .. _process: https://blog.vyos.io/vyos-development-digest-10 .. _vyos-1x: https://github.com/vyos/vyos-1x/blob/current/schema/ .. _VyConf: https://github.com/vyos/vyconf/blob/master/data/schemata - |