Commit c695388b authored by Grant Paton-Simpson's avatar Grant Paton-Simpson

Style customised linter messages:

* Add headings
* Allow custom content to include highlighted code and headings
* Display custom content first
parent 4a49e42d
......@@ -180,8 +180,9 @@ def if_else_overview(block_dets, *, repeat=False):
When using `if`, `elif`, or `else`, the item evaluated can
either be an expression or a name (variable). Sometimes your
code can be more readable if the expression is precalculated and
a name is supplied instead. For example:
code can be more readable if you make an intermediate variable
and use that instead. Shorter, more readable, easier to check
the parts. For example:
""")
+
......
from collections import defaultdict
from collections import defaultdict, namedtuple
from pathlib import Path
import re
from subprocess import run, PIPE
......@@ -11,6 +11,14 @@ from ..utils import get_nice_str_list, get_os_platform, \
prog = re.compile(lint_conf.LINT_PATTERN, flags=re.VERBOSE) # @UndefinedVariable
MsgDets = namedtuple('MsgDets', 'msg, line_no')
MISC_ISSUES_TITLE = layout("""\
#### Misc lint issues
""")
def _store_snippet(snippet):
tmp_fh, fpath = make_open_tmp_file(conf.SNIPPET_FNAME, mode='w')
tmp_fh.write(snippet)
......@@ -57,37 +65,52 @@ def _get_flake8_results(fpath):
return res
def _msg_type_to_placeholder_key(msg_type):
return f"{msg_type}_msg"
return f"{msg_type}_placeholder"
def _msg_type_to_placeholder(msg_type):
placeholder_key = _msg_type_to_placeholder_key(msg_type)
placeholder = '{' + placeholder_key + '}' ## ready for .format()
return placeholder
def _get_lint_dets_by_msg_type(lint_regex_dicts):
def _get_msg_type_and_dets(lint_regex_dicts):
"""
Gather by message type e.g. all the E123s together. Each may have messages
for multiple lines. These lines may have the same or different messages.
E.g. "line too long (91 > 79 characters)" vs "line too long (100 > 79
characters)".
Gather MsgDets named tuples by message type e.g. all the E123s together.
Some lint message types are consolidated as per
lint_conf.CONSOLIDATE_MSG_TYPE.
Each message type may have messages for multiple lines. These lines may have
the same or different messages. E.g. "line too long (91 > 79 characters)" vs
"line too long (100 > 79 characters)".
We may also want to supplement / replace the standard messages with special
messages. When replacing a message, we automatically want to consolidate to
one message. If not replacing the message, we want our special content to
appear after the unconsolidated messages for the message type.
Consolidation: for example, "line too long (91 > 79 characters)" and "line
too long (100 > 79 characters)" become "line too long", and there will be
one message with multiple line numbers listed rather than multiple messages,
one per line.
appear after the unconsolidated messages for the (possibly consolidated)
message type. We use placeholders to make this possible. These are replaced
later on when making message level-specific messages (brief and main only).
Message consolidation: for example, "line too long (91 > 79 characters)" and
"line too long (100 > 79 characters)" become "line too long", and there will
be one message with multiple line numbers listed rather than multiple
messages, one per line.
Note: just because a message type is consolidated doesn't mean its messages
will be consolidated or vice versa. They are independent transformations.
:param list lint_regex_dicts: list of dicts - one per regex.
Basically a list of pulled apart lint messages.
:return: dict of message types as keys (possibly consolidated e.g.
E123-9 -> line continuation message type) and MsgDets tuples as values.
Note - messages being replaced are consolidated into a placeholder.
:rtype: dict
"""
lint_dets_by_msg_type = defaultdict(list)
msg_type_and_dets = defaultdict(list)
for lint_regex_dict in lint_regex_dicts:
line_no = int(lint_regex_dict[conf.LINT_LINE_NO])
msg_type = lint_regex_dict[conf.LINT_MSG_TYPE]
msg = lint_regex_dict[conf.LINT_MSG]
raw_msg_type = lint_regex_dict[conf.LINT_MSG_TYPE]
msg_type = lint_conf.CONSOLIDATE_MSG_TYPE.get(
raw_msg_type, raw_msg_type)
msg = layout(lint_regex_dict[conf.LINT_MSG])
try:
msg_dets = lint_conf.CUSTOM_LINT_MSGS[msg_type]
except KeyError:
......@@ -102,59 +125,58 @@ def _get_lint_dets_by_msg_type(lint_regex_dicts):
## "{E123_msg} (lines 1 and 2)".format(E123_msg=e123_brief_msg)
msg_placeholder = _msg_type_to_placeholder(msg_type)
msg = msg_placeholder
msg_line_no_pair = (msg, line_no)
lint_dets_by_msg_type[msg_type].append(msg_line_no_pair)
return lint_dets_by_msg_type
msg_dets = MsgDets(msg, line_no)
msg_type_and_dets[msg_type].append(msg_dets)
return msg_type_and_dets
def _get_msg_line(msg_lineno_pairs):
def _get_msg_line(msgs_dets):
"""
Handle details for message type across all lines and specific messages.
Get a single message string. Handle details for message type across all
lines and specific messages.
:rtype: string
:return: a single line
"""
msg_type_details = []
msg_line_nos = defaultdict(set)
for msg, line_no in msg_lineno_pairs:
msg_line_nos[msg].add(line_no)
for msg_dets in msgs_dets:
msg_line_nos[msg_dets.msg].add(msg_dets.line_no)
for msg, line_nos in msg_line_nos.items():
plural = 's' if len(line_nos) > 1 else ''
nice_lines = get_nice_str_list(sorted(line_nos))
msg2use = msg[0].upper() + msg[1:] ## note - .capitalize() lower cases the remaining letters
msg2use = msg2use.replace('>', '\>').replace('<', '\<')
msg_type_details.append(f"{msg2use} (line{plural}:{nice_lines})")
msg_line = '; '.join(msg_type_details)
msg_line = layout('; '.join(msg_type_details))
return msg_line
def _get_msg_lines(lint_dets_by_msg_type):
msg_lines = []
def _get_unfinished_messages(msg_type_and_dets):
"""
Get list of unfinished messages. May contain placeholders that need to be
replaced.
"""
unfinished_msgs = []
already_supplemented = set()
for msg_type, msg_lineno_pairs in lint_dets_by_msg_type.items():
msg_line = _get_msg_line(msg_lineno_pairs)
msg_lines.append(msg_line)
for msg_type, msgs_dets in msg_type_and_dets.items():
unfinished_msg = _get_msg_line(msgs_dets)
generic_msg_type = msg_type not in lint_conf.CUSTOM_LINT_MSGS
if generic_msg_type:
unfinished_msg = '* ' + unfinished_msg.lstrip('\n') ## bullet points for generic messages (custom messages have full layout treatment e.g. code highlighting)
unfinished_msgs.append(unfinished_msg)
## add supplementary line?
supplement_configured = msg_type in lint_conf.CUSTOM_LINT_MSGS
if msg_type not in already_supplemented and supplement_configured:
msg_dets = lint_conf.CUSTOM_LINT_MSGS[msg_type]
supplement_needed = not msg_dets.replacement
msg_part_dets = lint_conf.CUSTOM_LINT_MSGS[msg_type]
supplement_needed = not msg_part_dets.replacement
if supplement_needed:
msg_placeholder = _msg_type_to_placeholder(msg_type)
msg_lines[-1] = msg_lines[-1] + ' ' + msg_placeholder ## will be replaced by appropriate level message in brief and main
last_msg = unfinished_msgs[-1]
last_msg = last_msg + '\n\n' + msg_placeholder ## will be replaced by appropriate level message in brief and main
already_supplemented.add(msg_type)
return msg_lines
def _get_lint_msg(raw_lint_message, msg_level):
"""
Make a dict mapping e.g. E501_msg to a message appropriate for the msg_level
return unfinished_msgs
msg_dets is a named tuple including brief, main fields
"""
msg_type_to_msg_level = {
_msg_type_to_placeholder_key(msg_type): getattr(msg_dets, msg_level)
for msg_type, msg_dets in lint_conf.CUSTOM_LINT_MSGS.items()
}
lint_msg = raw_lint_message.format(**msg_type_to_msg_level) ## e.g. E501_msg="Line too long", E666_msg='Code is generally evil'
return lint_msg
def _get_extra_msg(lint_dets_by_msg_type):
msg_types_used = lint_dets_by_msg_type.keys()
def _get_extra_msg(msg_type_and_dets):
msg_types_used = msg_type_and_dets.keys()
extra_lines = []
for msg_type in msg_types_used:
msgs = lint_conf.CUSTOM_LINT_MSGS.get(msg_type)
......@@ -164,6 +186,30 @@ def _get_extra_msg(lint_dets_by_msg_type):
extra_msg = '\n\n'.join(extra_lines)
return extra_msg
def _heading_sort_order(msg_line):
"""
Custom first (i.e. generic False first), then by message i.e. by title
"""
custom = ' #### ' in msg_line[: 10] ## Custom all start with #### ...
return (not custom, msg_line) ## for sorting - False is first
def _get_final_msgs_for_level(msgs_for_msg_level):
"""
Can only add Misc heading once everything is sorted by actual message
content (i.e. placeholders replaced and then sorted)
"""
final_msgs_for_level = []
has_generic_header = False
for msg_for_msg_level in msgs_for_msg_level:
starting_misc = '* ' in msg_for_msg_level[:5]
if not has_generic_header and starting_misc:
msgs2extend = [MISC_ISSUES_TITLE, msg_for_msg_level]
has_generic_header = True
else:
msgs2extend = [msg_for_msg_level, ]
final_msgs_for_level.extend(msgs2extend)
return final_msgs_for_level
def get_lint_messages_by_level(raw_lint_feedback_str):
"""
Gets lists of lint messages grouped by message level (brief, main, extra).
......@@ -188,19 +234,28 @@ def get_lint_messages_by_level(raw_lint_feedback_str):
for lint_line in lint_lines:
result = prog.match(lint_line)
lint_regex_dicts.append(result.groupdict())
## gather by msg_type e.g. E501s grouped together
lint_dets_by_msg_type = _get_lint_dets_by_msg_type(lint_regex_dicts)
## within message types gather by actual message (or msg_placeholder if we're replacing the standard message) and list line numbers
msg_lines = _get_msg_lines(lint_dets_by_msg_type)
## assemble
raw_lint_message = layout('* ' + '\n\n* '.join(msg_lines))
## misc
msg_type_and_dets = _get_msg_type_and_dets(lint_regex_dicts)
unfinished_messages = _get_unfinished_messages(msg_type_and_dets)
## replace placeholders with level-appropriate messages
## we can finally sort messages within a brief / main message level!
lint_msgs = []
for msg_level in ['brief', 'main']:
lint_msg = _get_lint_msg(raw_lint_message, msg_level)
lint_msgs.append(layout(lint_msg))
msg_type_to_msg_for_level = {
_msg_type_to_placeholder_key(msg_type): getattr(msg_dets, msg_level)
for msg_type, msg_dets in lint_conf.CUSTOM_LINT_MSGS.items()
}
msgs_for_level = [
unfinished_message.format(**msg_type_to_msg_for_level) ## maps msg_type e.g. E501 to e.g. "Line too long", E666_msg='Code is generally evil'
for unfinished_message in unfinished_messages]
msgs_for_level.sort(key=_heading_sort_order)
## Can only add Misc heading once everything is sorted by actual message
## content (i.e. placeholders replaced and then sorted)
final_msgs_for_level = _get_final_msgs_for_level(msgs_for_level)
final_msg_for_level = '\n\n'.join(final_msgs_for_level)
lint_msgs.append(final_msg_for_level)
## handle extra - see which placeholders are present and provide extra for those only
extra_msg = _get_extra_msg(lint_dets_by_msg_type)
extra_msg = _get_extra_msg(msg_type_and_dets)
lint_msgs.append(extra_msg)
return lint_msgs
......@@ -249,6 +304,8 @@ def lint_snippet(snippet):
raw_lint_feedback_str=res.stdout)
obviousness = layout("""\
#### Good code is simple enough to reason about
Linting is especially useful for an interpreted language like Python
because there is no compiler to pick up "lint" errors. Linting is no
substitute for unit testing though. And neither are a substitute for
......@@ -256,11 +313,12 @@ def lint_snippet(snippet):
single best protection against code not doing what it is meant to do.
The goal should be code where there is obviously nothing wrong rather
than code where there nothing obviously wrong.
> "There are two ways of constructing a software design. One way is to
make it so simple that there are obviously no deficiencies. And the
other way is to make it so complicated that there are no obvious
deficiencies." C.A.R. Hoare
""")
message = {
......
......@@ -27,14 +27,58 @@ else:
## All snippets here should be raw strings (see https://stackoverflow.com/questions/53636723/python-parsing-code-with-new-line-character-in-them-using-ast)
TEST_SNIPPET = r"""
def sorted(*G, **kwargs):
for i in range(len(G)):
for j in range(1,len(G)):
if G[j-1]<G[j]:
G[j-1],G[j]=G[j],G[j-1]
G = [['Ahmad', 3.8], ['Rizwan', 3.68], ['Bilal', 3.9]]
sorted(G)
print(G)
def function_with_really_long_name(parameter_1, parameter_2,
parameter_3):
pass
def function_with_really_long_name2(
parameter_1, parameter_2, parameter_3):
pass
def function_with_really_long_name3(
parameter_1,
parameter_2,
parameter_3):
pass
def function_with_really_long_name4(
parameter_1,
parameter_2,
parameter_3):
pass
def function_with_really_long_name5(
parameter_1,
parameter_2,
parameter_3):
pass
my_list = [
'a',
]
my_list2 = [
'a',
]
my_list3 = [
'a',
]
my_list4 = [
'a',
]
a = 'vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvaaaavvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv'
b = 'vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvbbvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv'
c = 'vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvcvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv'
func1("abc"
"def")
func2("abc"
"def")
func3("abc"
"def")
"""
PY3_6 = '3.6'
......@@ -105,6 +149,8 @@ METHOD_LBL = 'method'
EMAIL2USE = 'superhelp@p-s.co.nz'
INTRO = ("Help is provided for your overall snippet and for each block of code "
"as appropriate.")
NO_ADVICE_MESSAGE = ("No advice to give - looks fine :-). But if you think "
f"there should have been some advice given, contact {EMAIL2USE} "
"with the subject line 'Advice' and explain. Please include a snippet to "
......
......@@ -52,8 +52,7 @@ def display(snippet, messages_dets, *,
md2cli.main(layout(f"""\
# SuperHELP - Help for Humans!
Help is provided for your overall snippet and for each block of code
as appropriate.
{conf.INTRO}
Currently showing {message_level} content as requested.
......
......@@ -259,7 +259,7 @@ BROWSER_HTML_WRAPPER = """\
<body>
{logo_svg}
<h1>SuperHELP - Help for Humans!</h1>
<p>Help is provided for each block of code in your snippet.</p>
{intro}
<p>{missing_advice_message}</p>
<p>Toggle between different levels of detail.</p>
{radio_buttons}
......@@ -274,7 +274,7 @@ NOTEBOOK_HTML_WRAPPER = """\
{head}
<body>
<h1>Look here for some help on the snippet in the cell above</h1>
<p>Help is provided for each block of code in your snippet.</p>
{intro}
<p>{missing_advice_message}</p>
<p>Toggle between different levels of detail.</p>
{radio_buttons}
......@@ -659,6 +659,7 @@ def display(snippet, messages_dets, *,
:param bool in_notebook: if True, return HTML as string; else open browser
and display
"""
intro = f"<p>{conf.INTRO}</p>"
radio_buttons = _get_radio_buttons(message_level=message_level)
overall_messages_dets, block_messages_dets = messages_dets
all_html_strs = _get_all_html_strs(snippet,
......@@ -670,6 +671,7 @@ def display(snippet, messages_dets, *,
html2write = NOTEBOOK_HTML_WRAPPER.format(
head=head,
radio_buttons=radio_buttons,
intro=intro,
missing_advice_message=conf.MISSING_ADVICE_MESSAGE,
body_inner=body_inner,
visibility_script=VISIBILITY_SCRIPT)
......@@ -678,6 +680,7 @@ def display(snippet, messages_dets, *,
html2write = BROWSER_HTML_WRAPPER.format(
head=head, logo_svg=LOGO_SVG,
radio_buttons=radio_buttons,
intro=intro,
missing_advice_message=conf.MISSING_ADVICE_MESSAGE,
body_inner=body_inner,
visibility_script=VISIBILITY_SCRIPT)
......
from collections import namedtuple
from . import conf # @UnresolvedImport
from .utils import layout_comment as layout
## extract patterns e.g. from:
## /tmp/snippet.py:1:23: W291 trailing whitespace
## /tmp/snippet.py:4:1: E999 IndentationError: unexpected indent
LINT_PATTERN = fr"""^.+?: ## starts with misc until first colon e.g. /tmp/snippet.py: (+? to be non-greedy)
(?P<{conf.LINT_LINE_NO}>\d+) ## line_no = one or more digits after snippet.py: and before the next : e.g. 4
(?P<{conf.LINT_LINE_NO}>\d+) ## line_no = one or more digits after snippet.py: and before the next : e.g. 4 # @UndefinedVariable
:\d+:\s{{1}} ## one or more digits, :, and one space e.g. '1: '
(?P<{conf.LINT_MSG_TYPE}>\w{{1}}\d{{1,3}})\s{{1}} ## msg_type = (one letter, 1-3 digits) and one space e.g. E999
(?P<{conf.LINT_MSG_TYPE}>\w{{1}}\d{{1,3}})\s{{1}} ## msg_type = (one letter, 1-3 digits) and one space e.g. 'E999 '
(?P<{conf.LINT_MSG}>.*) ## msg = everything else e.g. 'IndentationError: unexpected indent'
"""
## https://pycodestyle.pycqa.org/en/latest/intro.html#error-codes
## https://flake8.pycqa.org/en/latest/user/error-codes.html
IGNORED_LINT_RULES = [
'E128', ## flake8 wants visual alignment on line continuation - I don't - I want standardised alignment on indent multiples of 4 (idea taken from Brandon Rhodes thanks Brandon!)
'E266', 'E262', ## I like ## before comments and # before commented out code (idea copied off Tom Eastman - thanks Tom!)
'E305', ## for classes I agree with 2 spaces but not functions
]
def title2msg_type(title):
return title.upper().replace(' ', '_') + '_MSG_TYPE'
## Ensure brief AND main are the same so titles don't shift when
## changing message level
line_continuation_title = "Line continuation"
line_length_title = "Line length"
unused_imports = "Unused imports"
## nice to keep placeholder names etc aligned with actual titles but nothing breaks if we don't
LINE_CONTINUATION_MSG_TYPE = title2msg_type(line_continuation_title)
LINE_LENGTH_MSG_TYPE = title2msg_type(line_length_title)
UNUSED_IMPORT_MSG_TYPE = title2msg_type(unused_imports)
CONSOLIDATE_MSG_TYPE = {
'E123': LINE_CONTINUATION_MSG_TYPE,
'E124': LINE_CONTINUATION_MSG_TYPE,
'E125': LINE_CONTINUATION_MSG_TYPE,
'E126': LINE_CONTINUATION_MSG_TYPE,
'E127': LINE_CONTINUATION_MSG_TYPE,
'E128': LINE_CONTINUATION_MSG_TYPE,
'E129': LINE_CONTINUATION_MSG_TYPE,
'E131': LINE_CONTINUATION_MSG_TYPE,
'E501': LINE_LENGTH_MSG_TYPE,
'F401': UNUSED_IMPORT_MSG_TYPE,
}
LintMsgs = namedtuple('LintMsgs', 'brief, main, extra, replacement')
CUSTOM_LINT_MSGS = {
'E501': LintMsgs(
"""\
One or more lines are longer than the recommended 79 characters. This is
not necessarily a problem but long lines should be an exception to the
rule
""",
"""\
One or more lines are longer than the recommended 79 characters. This is
not necessarily a problem given that we have wider monitors than when
the guidelines were formulated. But long lines should be an exception to
the rule. All being equal, short lines are easier to read and understand
than long lines. There are multiple strategies for shortening lines but
the overall goal has to be readability. Sometimes we have to live with
broken "rules". And that's official. Read PEP 8 - the official Python
style guide - especially the section "A Foolish Consistency is the
Hobgoblin of Little Minds".
""",
LINE_CONTINUATION_MSG_TYPE: LintMsgs(
layout(f"""
#### {line_continuation_title}
The linter has raised questions about line continuation. There are
at least two styles of line continuation. Whichever you follow be
consistent.
"""),
(
layout(f"""\
#### {line_continuation_title}
The linter has raised questions about line continuation. There
are at least two styles of line continuation:
1) Visual line continuation e.g. note how parameter_3 lines up
with the opening parenthesis
""")
+
layout("""\
def function_with_long_name(parameter_1, parameter_2,
parameter_3, parameter_4):
'''
Doc string for function
'''
""", is_code=True)
+
layout("""\
Pros:
* a common convention
Cons:
* every time you rename a function you have to realign
parameters
* code will generally not align tidily with multiples of
standard indentation
* room for parameters squeezed
* you have to look to the right to make sense of the function
2) Leftwards alignment with multiples of standard indentation
e.g.
""")
+
layout("""\
def function_with_long_name(parameter_1, parameter_2,
parameter_3, parameter_4):
'''
Doc string for function
'''
""", is_code=True)
+
layout("""\
or
""")
+
layout("""\
def function_with_long_name(
parameter_1, parameter_2, parameter_3, parameter_4):
'''
Doc string for function
'''
""", is_code=True)
+
layout("""\
or
""")
+
layout("""\
def function_with_long_name(
parameter_1,
parameter_2,
parameter_3,
parameter_4):
'''
Doc string for function
'''
""", is_code=True)
+
layout("""\
Pros:
* readability is enhanced because the main content of the code
is as leftward as possible
* beautiful alignment across entire module
Cons:
* you may need to change your IDE's default indenting behaviour
Whichever of the two style you follow, be consistent.
""")
),
layout("""\
See <https://rhodesmill.org/brandon/slides/2012-11-pyconca/#id183>
for some thought-provoking ideas on code style and much more.
"""),
True),
LINE_LENGTH_MSG_TYPE: LintMsgs(
layout(f"""\
#### {line_length_title}
One or more lines are longer than the recommended 79 characters.
This is not necessarily a problem but long lines should be an
exception to the rule.
"""),
layout(f"""\
#### {line_length_title}
One or more lines are longer than the recommended 79 characters.
This is not necessarily a problem given that we have wider monitors
than when the guidelines were formulated. But long lines should be
an exception to the rule. All being equal, short lines are easier to
read and understand than long lines. There are multiple strategies
for shortening lines but the overall goal has to be readability.
Sometimes we have to live with broken "rules". And that's official.
Read PEP 8 - the official Python style guide - especially the
section "A Foolish Consistency is the Hobgoblin of Little Minds".
"""),
'',
True),
'F401': LintMsgs(
"""\
One or more imports not used in snippet.
""",
"""\
One or more imports not used in snippet. If the snippet was extracted
from a larger piece of code and the imports are used in that code then
there is no problem.
""",
UNUSED_IMPORT_MSG_TYPE: LintMsgs(
layout(f"""\
#### {unused_imports}
One or more imports not used in snippet.
"""),
layout(f"""\
#### {unused_imports}
One or more imports not used in snippet. If the snippet was
extracted from a larger piece of code and the imports are used in
that code then there is no problem.
"""),
'',
False