-
Notifications
You must be signed in to change notification settings - Fork 218
Add style tests #1618
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
Add style tests #1618
Changes from 25 commits
06a5646
dd95739
276f17a
9e92dab
d482ff2
adac638
9056d23
fbac45a
6e62431
613fdcf
286c0e9
4604134
04548c9
d434485
fce593f
215f7ad
34315fa
74a8cf4
41651bf
529af5c
a7991b4
210cc17
5a51dee
e1bf239
854f1dc
5e047ca
d92bb09
a12c72c
795048f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,7 +43,7 @@ before_install: | |
| # pydot (dep for python-graph-dot) 1.2.0 and more recent doesn't work with Python 2.6 | ||
| - if [ "x$TRAVIS_PYTHON_VERSION" == 'x2.6' ]; then pip install pydot==1.1.0; else pip install pydot; fi | ||
| # optional Python packages for EasyBuild | ||
| - pip install autopep8 GC3Pie python-graph-dot python-hglib PyYAML | ||
| - pip install pep8 autopep8 GC3Pie python-graph-dot python-hglib PyYAML | ||
| # git config is required to make actual git commits (cfr. tests for GitRepository) | ||
| - git config --global user.name "Travis CI" | ||
| - git config --global user.email "[email protected]" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| ## | ||
| # Copyright 2016-2016 Ghent University | ||
| # | ||
| # This file is part of EasyBuild, | ||
| # originally created by the HPC team of Ghent University (http://ugent.be/hpc/en), | ||
| # with support of Ghent University (http://ugent.be/hpc), | ||
| # the Flemish Supercomputer Centre (VSC) (https://www.vscentrum.be), | ||
| # Flemish Research Foundation (FWO) (http://www.fwo.be/en) | ||
| # and the Department of Economy, Science and Innovation (EWI) (http://www.ewi-vlaanderen.be/en). | ||
| # | ||
| # http://github.com/hpcugent/easybuild | ||
| # | ||
| # EasyBuild is free software: you can redistribute it and/or modify | ||
| # it under the terms of the GNU General Public License as published by | ||
| # the Free Software Foundation v2. | ||
| # | ||
| # EasyBuild 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 EasyBuild. If not, see <http://www.gnu.org/licenses/>. | ||
| # # | ||
|
|
||
| """ | ||
| Style tests for easyconfig files using pep8. | ||
| :author: Ward Poelmans (Ghent University) | ||
| """ | ||
|
|
||
| import re | ||
| from vsc.utils import fancylogger | ||
|
|
||
| from easybuild.tools.utilities import only_if_module_is_available | ||
|
|
||
| try: | ||
| import pep8 | ||
| except ImportError: | ||
| pass | ||
|
|
||
| _log = fancylogger.getLogger('easyconfig.style', fname=False) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why add a logger if you're not using it? (don't remove it, use it below :)) |
||
|
|
||
|
|
||
| # any function starting with _eb_check_ will be added to the tests | ||
| # if the test number is added to the select list. The test number is | ||
| # definied as WXXX and EXXX (for warnings and errors) where XXX is a | ||
| # 3 digit number. | ||
| # | ||
| # It should be mentioned in the docstring as a single word. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. on a separate line? |
||
| # Read the pep8 docs to understand the arguments of these functions: | ||
| # https://pep8.readthedocs.org or more specifically: | ||
| # https://pep8.readthedocs.org/en/latest/developer.html#contribute | ||
| def _eb_check_trailing_whitespace(physical_line, lines, line_number, total_lines): # pylint:disable=unused-argument | ||
| """ | ||
| W299 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why shouldn't be define our own range like
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, there is a structure in the codes: https://pep8.readthedocs.org/en/latest/intro.html#error-codes |
||
| Warn about trailing whitespace, except for the description and comments. | ||
| This differs from the standard trailing whitespace check as that | ||
| will warn for any trailing whitespace. | ||
| The arguments are explained at | ||
| https://pep8.readthedocs.org/en/latest/developer.html#contribute | ||
| """ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. doc args?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can copy & paste the pep8 docs but I prefer not to. The names are already quite clear and the precise meaning should be looked up in the url above
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please refer to the right place in the pep8 docs in the docstring; the comments won't show up in the auto-generated API documentation for EB we'll have at easybuild.readthedocs.org someday... |
||
| comment_re = re.compile(r'^\s*#') | ||
| if comment_re.match(physical_line): | ||
| return None | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not a big fan of inline returns (we rarely do this in the current codebase, I'd prefer sticking to that)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's much more readable in this way. Inline returns are not bad. |
||
|
|
||
| result = pep8.trailing_whitespace(physical_line) | ||
| if result: | ||
| result = (result[0], result[1].replace("W291", "W299")) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ideally, we only have the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it should be in the docs as well, so it's gonna be here twice.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we string template a docstring? :P
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just tested this, templating on a docstring doesn't really work... |
||
|
|
||
| # if the warning is about the multiline string of description | ||
| # we will not issue a warning | ||
| keys_re = re.compile(r"^(?P<key>[a-z_]+)\s*=\s*") | ||
|
|
||
| # starting from the current line and going to the top, | ||
| # check that if the first `key = value` that is found, has | ||
| # key == description, then let the test pass, else return | ||
| # the result of the general pep8 check. | ||
| for line in reversed(lines[:line_number]): | ||
| res = keys_re.match(line) | ||
| if res: | ||
| if res.group("key") == "description": | ||
| return None | ||
| else: | ||
| break | ||
|
||
|
|
||
| return result | ||
|
|
||
|
|
||
| @only_if_module_is_available('pep8') | ||
| def style_conformance(easyconfigs, verbose=False): | ||
|
||
| """ | ||
| Check the given list of easyconfigs for style | ||
| :param easyconfigs list of file paths to easyconfigs | ||
| :param verbose print our statistics and be verbose about the errors and warning | ||
|
||
| :return the number of warnings and errors | ||
|
||
| """ | ||
| # importing autopep8 changes some pep8 functions. | ||
| # We reload it to be sure to get the real pep8 functions. | ||
| reload(pep8) | ||
|
|
||
| # register the extra checks before using pep8: | ||
| # any function in this module starting with `_eb_check_` will be used. | ||
| cands = globals() | ||
| for check_function in sorted([cands[f] for f in cands if callable(cands[f]) and f.startswith('_eb_check_')]): | ||
|
||
| _log.debug("Adding custom style check %s", check_function) | ||
| pep8.register_check(check_function) | ||
|
|
||
| pep8style = pep8.StyleGuide(quiet=False, config_file=None) | ||
| options = pep8style.options | ||
| # we deviate from standard pep8 and allow 120 chars | ||
| # on a line: the default of 79 is too narrow. | ||
| options.max_line_length = 120 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add a comment above to explain why (and what the default is) |
||
| # we ignore some tests | ||
| # note that W291 has be replaced by our custom W299 | ||
| options.ignore = ( | ||
| 'E402', # import not on top | ||
| 'W291', # replaced by W299 | ||
| 'E501', # line too long | ||
|
||
| ) | ||
| options.verbose = int(verbose) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just make the default value
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it is 0? |
||
|
|
||
| result = pep8style.check_files(easyconfigs) | ||
|
|
||
| if verbose: | ||
| result.print_statistics() | ||
|
|
||
| return result.total_errors | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,29 +1,34 @@ | ||
| # should be EB_GCC, but OK for testing purposes | ||
| easyblock = 'EB_toy' | ||
|
|
||
| name="GCC" | ||
| version='4.6.3' | ||
| name = "GCC" | ||
| version = '4.6.3' | ||
|
|
||
| homepage='http://gcc.gnu.org/' | ||
| description="The GNU Compiler Collection includes front ends for C, C++, Objective-C, Fortran, Java, and Ada, as well as libraries for these languages (libstdc++, libgcj,...)." | ||
| homepage = 'http://gcc.gnu.org/' | ||
| description = """The GNU Compiler Collection includes front ends for C, C++, Objective-C, Fortran, Java, and Ada, | ||
| as well as libraries for these languages (libstdc++, libgcj,...).""" | ||
|
|
||
| toolchain={'name': 'dummy','version': 'dummy'} | ||
| toolchain = {'name': 'dummy', 'version': 'dummy'} | ||
|
|
||
| sources=['%s-%s.tar.gz'%(name.lower(),version), | ||
| 'gmp-5.0.4.tar.bz2', | ||
| 'mpfr-3.0.1.tar.gz', | ||
| 'mpc-0.9.tar.gz', | ||
| ] | ||
| source_urls=['http://ftpmirror.gnu.org/%(name)s/%(name)s-%(version)s' % {'name':name.lower(), 'version':version}, # GCC auto-resolving HTTP mirror | ||
| 'http://ftpmirror.gnu.org/gmp', # idem for GMP | ||
| 'http://ftpmirror.gnu.org/mpfr', # idem for MPFR | ||
| 'http://www.multiprecision.org/mpc/download', # MPC official | ||
| ] | ||
| sources = [ | ||
| '%s-%s.tar.gz' % (name.lower(), version), | ||
| 'gmp-5.0.4.tar.bz2', | ||
| 'mpfr-3.0.1.tar.gz', | ||
| 'mpc-0.9.tar.gz', | ||
| ] | ||
|
|
||
| languages=['c', 'c++', 'fortran', 'lto'] | ||
| source_urls = [ | ||
| # GCC auto-resolving HTTP mirror | ||
| 'http://ftpmirror.gnu.org/%(name)s/%(name)s-%(version)s' % {'name': name.lower(), 'version': version}, | ||
| 'http://ftpmirror.gnu.org/gmp', # idem for GMP | ||
| 'http://ftpmirror.gnu.org/mpfr', # idem for MPFR | ||
| 'http://www.multiprecision.org/mpc/download', # MPC official | ||
| ] | ||
|
|
||
| ## compiler class | ||
| moduleclass='compiler' | ||
| languages = ['c', 'c++', 'fortran', 'lto'] | ||
|
|
||
| # compiler class | ||
| moduleclass = 'compiler' | ||
|
|
||
| # building GCC sometimes fails if make parallelism is too high, so let's limit it | ||
| maxparallel=4 | ||
| maxparallel = 4 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,8 @@ | |
| easyblock = 'ConfigureMake' | ||
|
|
||
| name = 'gzip' | ||
| version = '1.6' # FIXME bug: quotes are required here to make sure this is parsed a a string, not a floating point value | ||
| # FIXME bug: quotes are required here to make sure this is parsed a a string, not a floating point value | ||
|
||
| version = '1.6' | ||
|
|
||
| homepage = 'http://www.gnu.org/software/gzip/' | ||
| description = "gzip (GNU zip) is a popular data compression program as a replacement for compress" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nitpicking: retain alphabetical order