Conversation
hustlijian
commented
Nov 16, 2018
hustlijian
commented
- set windows cache directory
- splint lines
- use which
|
@hustlijian thanks for the pr. I left some comments. Something that's missing though is to configure CI to run on windows. |
.travis.yml
Outdated
| install: | ||
| - pip install .[dev,test] coveralls | ||
| - bash scripts/install-linters.sh | ||
| - pip install -r requirements-dev.txt |
There was a problem hiding this comment.
This should not be necessary as the pip command above is also installing dev and test.
gitlint/__init__.py
Outdated
|
|
||
| import docopt | ||
| import termcolor | ||
| import colorama |
There was a problem hiding this comment.
I'd prefer colorama to be a dependency only for windows systems.
gitlint/utils.py
Outdated
|
|
||
| # This can be just pathlib when 2.7 and 3.4 support is dropped. | ||
| import pathlib2 as pathlib | ||
| from shutil import which |
gitlint/utils.py
Outdated
| def _get_cache_filename(name, filename): | ||
| """Returns the cache location for filename and linter name.""" | ||
| filename = os.path.abspath(filename)[1:] | ||
| filename = filename.lstrip(":\\") # for windows |
There was a problem hiding this comment.
Doesn't a drive letter appears before :\?
There was a problem hiding this comment.
removed by filename = os.path.abspath(filename)[1:]
requirements-dev.txt
Outdated
| @@ -0,0 +1,7 @@ | |||
| termcolor | |||
There was a problem hiding this comment.
Termcolor and colorama are not dev dependencies. But as said above, colorama should only be required for windows.
There was a problem hiding this comment.
This file should be removed an instead add the dependencies to the setup file. But colorama should only be required for windows.
See https://hynek.me/articles/conditional-python-dependencies/
There was a problem hiding this comment.
ok， i will change them to use setup.py, declaring-platform-specific-dependencies
.travis.yml
Outdated
| language: python | ||
| python: | ||
| - 2.7 | ||
| # - 2.7 |
There was a problem hiding this comment.
@hustlijian It's not acceptable to get rid of the Python 2.7 support.
|
@sk- I changed some code, and got python2.7/3.4 ci passed: travis build#9 , but two pyfakefs test can not passed, Can you help with it? |
sk-
left a comment
sk-
left a comment
There was a problem hiding this comment.
No idea about how to fix the tests, but that seems related to your changes as I just tested master and the tests still pass.
Also in the PR you need to enable testing in windows in the Travis config. See https://blog.travis-ci.com/2018-10-11-windows-early-release
requirements-dev.txt
Outdated
| @@ -0,0 +1,7 @@ | |||
| termcolor | |||
There was a problem hiding this comment.
This file should be removed an instead add the dependencies to the setup file. But colorama should only be required for windows.
See https://hynek.me/articles/conditional-python-dependencies/
gitlint/utils.py
Outdated
| import pathlib2 as pathlib | ||
|
|
||
|
|
||
| # copy from python3, shutil.which |
There was a problem hiding this comment.
Which version of python was used? Which commit?
Probably is better to depend on a backported version of which like. https://pypi.org/project/backports.shutil_which/
There was a problem hiding this comment.
Thanks for your patience, I will improve this PR later。
The language 'python' is currently unsupported on the Windows Build Environment. Let us know if you'd like to see it: https://travis-ci.community/c/environments/windows. Thanks for understanding!
2 participants
