Skip to content
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

Output options #28

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 36 additions & 1 deletion lib/ansiblereview/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@
from appdirs import AppDirs
from pkg_resources import resource_filename

from copy import copy
from optparse import Option, OptionValueError


def get_candidates_from_diff(difftext):
try:
Expand All @@ -31,12 +34,25 @@ def get_candidates_from_diff(difftext):
return candidates


def check_boolean(option, opt, value):
try:
return value.lower() in ("yes", "true", "t", "1")
except ValueError:
raise OptionValueError(
"option %s: invalid boolean value: %r" % (opt, value))

class MyOption (Option):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I did originally, but that doesn't work the way I expected: it forces you to have two parameters. So instead of looking like this:

--wrap_long_lines=True
--wrap_long_lines=False

it would turn into two options, like this:

--wrap_long_lines_on
--wrap_long_lines_off

If you'd rather do it that way, I can change it.

Copy link
Owner

@willthames willthames Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming it defaults to False, it should just be

parser.add_option('--wrap_long_lines', dest='wrap_long_lines', action="store_true", default=False,
                  help="Wrap long lines of output, or output each message on a single line.")

TYPES = Option.TYPES + ("boolean",)
TYPE_CHECKER = copy(Option.TYPE_CHECKER)
TYPE_CHECKER["boolean"] = check_boolean


def main():
config_dir = AppDirs("ansible-review", "com.github.willthames").user_config_dir
default_config_file = os.path.join(config_dir, "config.ini")

parser = optparse.OptionParser("%prog playbook_file|role_file|inventory_file",
version="%prog " + __version__)
version="%prog " + __version__, option_class=MyOption)
parser.add_option('-c', dest='configfile', default=default_config_file,
help="Location of configuration file: [%s]" % default_config_file)
parser.add_option('-d', dest='rulesdir',
Expand All @@ -50,6 +66,22 @@ def main():
parser.add_option('-v', dest='log_level', action="store_const", default=logging.WARN,
const=logging.INFO, help="Show more verbose output")

parser.add_option('--output_info', dest='output_info',
help="Where to output INFO messages: stdout|stderr")
parser.add_option('--output_warn', dest='output_warn',
help="Where to output WARN messages: stdout|stderr")
parser.add_option('--output_error', dest='output_error',
help="Where to output ERROR messages: stdout|stderr")
parser.add_option('--output_fatal', dest='output_fatal',
help="Where to output FATAL messages: stdout|stderr")

parser.add_option('--wrap_long_lines', dest='wrap_long_lines', type="boolean",
help="Wrap long lines of output, or output each message on a single line.")

parser.add_option('--print_options', dest='print_options', type="boolean", default=False,
help="Print out effective config options, before starting.")


options, args = parser.parse_args(sys.argv[1:])
settings = read_config(options.configfile)

Expand All @@ -72,6 +104,9 @@ def main():
warn("Using example lint-rules found at %s" % lint_dir, options, file=sys.stderr)
options.lintdir = lint_dir

if options.print_options:
print(str(options))

if len(args) == 0:
candidates = get_candidates_from_diff(sys.stdin)
else:
Expand Down
72 changes: 53 additions & 19 deletions lib/ansiblereview/utils/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,29 @@
import configparser


def abort(message, file=sys.stderr):
def abort(message, settings, file=None):
if file is None:
file = getattr(sys, settings.output_abort)
print(stringc("FATAL: %s" % message, 'red'), file=file)
sys.exit(1)


def error(message, file=sys.stderr):
def error(message, settings, file=None):
if file is None:
file = getattr(sys, settings.output_error)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to this much effort, should we support actual files too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe?

I'm post processing the output (#22) to turn it into a checklist - which was why I added the wrap_long_lines parameter. But I'm OK with ansible-review outputting to stdout, so the output can be feed to wherever.

I just added these lines so I could (optionally) have all the output go to stdout without having to bash redirecting it using 2>&1 or &>, because that was garbling it (stderr output in the middle of stdout lines, occasionally).

Maybe we could expand this to support outputting to regular files later?

print(stringc("ERROR: %s" % message, 'red'), file=file)


def warn(message, settings, file=sys.stdout):
def warn(message, settings, file=None):
if file is None:
file = getattr(sys, settings.output_warn)
if settings.log_level <= logging.WARNING:
print(stringc("WARN: %s" % message, 'yellow'), file=file)


def info(message, settings, file=sys.stdout):
def info(message, settings, file=None):
if file is None:
file = getattr(sys, settings.output_info)
if settings.log_level <= logging.INFO:
print(stringc("INFO: %s" % message, 'green'), file=file)

Expand All @@ -61,12 +69,12 @@ def is_line_in_ranges(line, ranges):

def read_standards(settings):
if not settings.rulesdir:
abort("Standards directory is not set on command line or in configuration file - aborting")
abort("Standards directory is not set on command line or in configuration file - aborting", settings)
sys.path.append(os.path.abspath(os.path.expanduser(settings.rulesdir)))
try:
standards = importlib.import_module('standards')
except ImportError as e:
abort("Could not import standards from directory %s: %s" % (settings.rulesdir, str(e)))
abort("Could not import standards from directory %s: %s" % (settings.rulesdir, str(e)), settings)
return standards


Expand Down Expand Up @@ -112,6 +120,11 @@ def review(candidate, settings, lines=None):
(type(candidate).__name__, candidate.path, candidate.version),
settings)

if settings.wrap_long_lines:
br = '\n'
else:
br = ' '

for standard in standards.standards:
if type(candidate).__name__.lower() not in standard.types:
continue
Expand All @@ -122,14 +135,14 @@ def review(candidate, settings, lines=None):
if not err.lineno or
is_line_in_ranges(err.lineno, lines_ranges(lines))]:
if not standard.version:
warn("Best practice \"%s\" not met:\n%s:%s" %
(standard.name, candidate.path, err), settings)
warn("Best practice \"%s\" not met:%s%s:%s" %
(standard.name, br, candidate.path, err), settings)
elif LooseVersion(standard.version) > LooseVersion(candidate.version):
warn("Future standard \"%s\" not met:\n%s:%s" %
(standard.name, candidate.path, err), settings)
warn("Future standard \"%s\" not met:%s%s:%s" %
(standard.name, br, candidate.path, err), settings)
else:
error("Standard \"%s\" not met:\n%s:%s" %
(standard.name, candidate.path, err))
error("Standard \"%s\" not met:%s%s:%s" %
(standard.name, br, candidate.path, err), settings)
errors = errors + 1
if not result.errors:
if not standard.version:
Expand All @@ -142,23 +155,44 @@ def review(candidate, settings, lines=None):
return errors


# TODO: Settings & read_config should probably just use:
#
# config.read(...)
# config._sections - which is a dict of the config file contents
class Settings(object):
def __init__(self, values):
self.rulesdir = values.get('rulesdir')
self.lintdir = values.get('lintdir')
self.rulesdir = values.get('rulesdir', None)
self.lintdir = values.get('lintdir', None)

self.configfile = values.get('configfile')

self.output_info = values.get('output_info', 'stdout')
self.output_warn = values.get('output_warn', 'stdout')
self.output_error = values.get('output_error', 'stderr')
self.output_fatal = values.get('output_fatal', 'stderr')

self.wrap_long_lines = values.get('wrap_long_lines', True) == 'True'


def read_config(config_file):
config = configparser.RawConfigParser({'standards': None, 'lint': None})
config.read(config_file)

tmp_settings = dict(configfile=config_file)

if config.has_section('rules'):
return Settings(dict(rulesdir=config.get('rules', 'standards'),
lintdir=config.get('rules', 'lint'),
configfile=config_file))
else:
return Settings(dict(rulesdir=None, lintdir=None, configfile=config_file))
tmp_settings['rulesdir'] = config.get('rules', 'standards')
tmp_settings['lintdir'] = config.get('rules', 'lint')

if config.has_section('output'):
tmp_settings['output_info'] = config.get('output', 'info')
tmp_settings['output_warn'] = config.get('output', 'warn')
tmp_settings['output_error'] = config.get('output', 'error')
tmp_settings['output_fatal'] = config.get('output', 'fatal')

tmp_settings['wrap_long_lines'] = config.get('output', 'wrap_long_lines')

return Settings(tmp_settings)


class ExecuteResult(object):
Expand Down
2 changes: 1 addition & 1 deletion lib/ansiblereview/version.py
Original file line number Diff line number Diff line change
@@ -1 +1 @@
__version__ = '0.13.2'
__version__ = '0.13.3'