Added Python 3 support #31
Conversation
|
Hey Marcus, Thanks for this. There seem to be quite a few changes here that don't readily appear relevant to Python 3 support. Could you clarify what needed changing for Python 3? Regarding the pytest error; it is saying that an expected case is failing and it seems to be because a |
| @@ -21,6 +21,7 @@ pip-log.txt | |||
|
|
|||
| # Unit test / coverage reports | |||
| .coverage | |||
| .cache | |||
There was a problem hiding this comment.
This was created by unit tests in Python 3.
| from .error import ParseError, FormatError, NotFound | ||
|
|
||
|
|
||
| def discover_templates(paths=None, recursive=True): |
There was a problem hiding this comment.
These were moved into template.py for consistency.
There was a problem hiding this comment.
I actually had these in __init__.py as the 'high level' API that operates on template instances rather than being templates themselves.
So I don't really see a benefit to changing this at this stage.
| get_template | ||
| ) | ||
|
|
||
| __all__ = [ |
There was a problem hiding this comment.
This was added for PEP08, and the "unused variables" warning.
There was a problem hiding this comment.
Hmm. My understanding is that __all__ is only required when the package author wants to control behaviour of from package import *, which is considered bad practice anyway.
As I have no need to control this (the default behaviour is fine), I am not clear on the benefit of adding this here. Could you explain further perhaps?
Also, I don't get any "unused variables" warning - what are you using to lint / check the code?
| import functools | ||
| from collections import defaultdict | ||
|
|
||
| import lucidity.error |
There was a problem hiding this comment.
This was changed to relative, to support vendoring.
There was a problem hiding this comment.
I deliberately favour absolute imports these days as they keep the namespacing visible and improve clarity.
By 'vendoring' I presume you mean including a package inside another package?
| @@ -50,7 +50,7 @@ def test_discover_with_env(path, expected, monkeypatch): | |||
| '''Discover templates using environment variable.''' | |||
| monkeypatch.setenv('LUCIDITY_TEMPLATE_PATH', path) | |||
| templates = lucidity.discover_templates() | |||
| assert map(operator.attrgetter('name'), templates) == expected | |||
| assert [template.name for template in templates] == expected | |||
| @@ -119,7 +119,7 @@ def test_non_matching_parse(pattern, path, template_resolver): | |||
| '''Extract data from non-matching path.''' | |||
| template = Template('test', pattern, template_resolver=template_resolver) | |||
| with pytest.raises(ParseError): | |||
| data = template.parse(path) | |||
| @@ -252,7 +258,7 @@ def _construct_regular_expression(self, pattern): | |||
| else: | |||
| _, value, traceback = sys.exc_info() | |||
| message = 'Invalid pattern: {0}'.format(value) | |||
| raise ValueError, message, traceback #@IgnorePep8 | |||
| raise ValueError(message, traceback) | |||
There was a problem hiding this comment.
I think this would need a Python 2 / 3 switch.
Note: The current syntax is required as a workaround for exception chaining (to maintain original traceback information). I believe Python 3 supports this as raise from.
|
Hello! Just wondering about the status of this pull request. |
Hey Martin,
I've added support for Python 3 and tidied up a few PEP8 issues. Apologies for not merge requesting on GitLab, I'm not as familiar with it as GitHub.
There's just one issue left that I'm having trouble parsing, possibly due to unfamiliarity with pytest. See here:
Does it make sense to you?