catkin / xylem

A tool for resolving dependencies in a platform agnostic way.
Apache License 2.0
1 stars 1 forks source link

Fix relative path to only test xylem source directory. #5

Closed NikolausDemmel closed 10 years ago

NikolausDemmel commented 10 years ago

Tests are executed in the root directry, which means e.g. 'docs' is currently also checked for pep8 conformance.

NikolausDemmel commented 10 years ago

One question about pep8 conformance: Are you generally using 120 char wide lines? Would be fine with me, just checking though.

NikolausDemmel commented 10 years ago

And what about width of docstrings?

wjwwood commented 10 years ago

Ah, that code is naive, it should be ../xylem from the location of this file, something more like this:

https://github.com/wjwwood/osrf_pycommon/blob/master/tests/test_code_format.py

In fact you can probably just copy that whole file and update the path, since it actually uses flake8.

W.r.t. the line length, we had conceded to use 120 characters as a team, but I am actually in favor of keeping everything down to 80 and for our new work we have said an 80 characters limit should be used.

I'll leave this up to your discretion. What ever you pick should also apply to docstrings in files. If you want to limit to 80 characters in the source, but find that limiting for docstrings you can write the longer module level documentation in the corresponding .rst file instead, which is not subject to PEP8. For example you could put your main docs in a file like this one:

https://raw.githubusercontent.com/wjwwood/osrf_pycommon/terminal_color/docs/terminal_color.rst

Rather then in the docstring of the module like I have done here:

https://github.com/wjwwood/osrf_pycommon/blob/terminal_color/osrf_pycommon/terminal_color/__init__.py#L15-124

NikolausDemmel commented 10 years ago

I have spent quite some (way too much) time reading into style guides and peps as well as tool support (flake8, pep8, pep257, pep8-naming, flake8-docstrings, ...). I will stick with 79 width for code and 72 for text (multiline docstrings, comments and also .rst files etc) as recommended by PEP 8. The https://github.com/ehuss/Sublime-Wrap-Plus package and the following settings work nicely for me:

    "rulers": [72, 79],
    "wrap_width": 72,
    "word_wrap": false

Also, for the nose tests we have flake8 checking (only default extensions) and I use the flake8 and pep257 linter in Sublime.

wjwwood commented 10 years ago

+1