search

pycontribs / jenkinsapi

A Python API for accessing resources and configuring Hudson & Jenkins continuous-integration servers
http://pypi.python.org/pypi/jenkinsapi
MIT License
858 stars 485 forks source link

simplify crumb usage #704

Closed JesperMonsted closed 5 years ago

JesperMonsted commented 5 years ago

Overly complicated to use crumb, having to duplicate requester parameters, this should make it easier for newcomers to utilize crumb while supporting backwards compatibility.

JesperMonsted commented 5 years ago

705 will help CI pass, we should not run codestyle on generated files like eggs:

py27 run-test: commands[1] | python -m pycodestyle
./.eggs/pbr-5.2.0-py2.7.egg/pbr/builddoc.py:135:13: E117 over-indented
./.eggs/pbr-5.2.0-py2.7.egg/pbr/git.py:159:29: W605 invalid escape sequence '\*'
./.eggs/pbr-5.2.0-py2.7.egg/pbr/git.py:160:29: W605 invalid escape sequence '\_'
./.eggs/pbr-5.2.0-py2.7.egg/pbr/git.py:161:29: W605 invalid escape sequence '\`'
./.eggs/pbr-5.2.0-py2.7.egg/pbr/packaging.py:306:1: E305 expected 2 blank lines after class or function definition, found 1
./.eggs/pbr-5.2.0-py2.7.egg/pbr/packaging.py:630:1: E305 expected 2 blank lines after class or function definition, found 1
./.eggs/pbr-5.2.0-py2.7.egg/pbr/util.py:211:13: E117 over-indented
./.eggs/pbr-5.2.0-py2.7.egg/pbr/util.py:213:13: E117 over-indented
./.eggs/pbr-5.2.0-py2.7.egg/pbr/tests/test_packaging.py:111:44: W605 invalid escape sequence '\s'
./.eggs/pbr-5.2.0-py2.7.egg/pbr/tests/test_packaging.py:111:48: W605 invalid escape sequence '\s'
./.eggs/pbr-5.2.0-py2.7.egg/pbr/tests/test_packaging.py:111:52: W605 invalid escape sequence '\d'
./.eggs/pbr-5.2.0-py2.7.egg/pbr/tests/test_packaging.py:111:57: W605 invalid escape sequence '\.'
./.eggs/pbr-5.2.0-py2.7.egg/pbr/tests/test_packaging.py:111:61: W605 invalid escape sequence '\d'
./.eggs/pbr-5.2.0-py2.7.egg/pbr/tests/test_packaging.py:111:66: W605 invalid escape sequence '\.'
./.eggs/pbr-5.2.0-py2.7.egg/pbr/tests/test_packaging.py:111:70: W605 invalid escape sequence '\d'
./.eggs/pbr-5.2.0-py2.7.egg/pbr/tests/test_packaging.py:296:24: W605 invalid escape sequence '\*'
./.eggs/pbr-5.2.0-py2.7.egg/pbr/tests/test_packaging.py:303:26: W605 invalid escape sequence '\_'
./.eggs/pbr-5.2.0-py2.7.egg/pbr/tests/test_packaging.py:304:26: W605 invalid escape sequence '\_'
./.eggs/pbr-5.2.0-py2.7.egg/pbr/tests/test_packaging.py:305:28: W605 invalid escape sequence '\_'
./.eggs/pbr-5.2.0-py2.7.egg/pbr/tests/test_packaging.py:312:24: W605 invalid escape sequence '\`'
./.eggs/pbr-5.2.0-py2.7.egg/pbr/tests/test_packaging.py:756:9: E731 do not assign a lambda expression, use a def
./.eggs/pbr-5.2.0-py2.7.egg/pbr/tests/test_setup.py:99:1: E305 expected 2 blank lines after class or function definition, found 1
./.eggs/pbr-5.2.0-py2.7.egg/pbr/tests/test_setup.py:128:1: E305 expected 2 blank lines after class or function definition, found 1
./.eggs/pbr-5.2.0-py2.7.egg/pbr/tests/test_setup.py:165:33: W605 invalid escape sequence '\_'
./.eggs/pbr-5.2.0-py2.7.egg/pbr/tests/test_setup.py:179:32: W605 invalid escape sequence '\_'
./.eggs/pbr-5.2.0-py2.7.egg/pbr/tests/test_wsgi.py:101:39: W605 invalid escape sequence '\d'
./.eggs/pbr-5.2.0-py2.7.egg/pbr/tests/util.py:56:13: E117 over-indented
./.eggs/pbr-5.2.0-py2.7.egg/pbr/tests/util.py:58:13: E117 over-indented
./.eggs/pbr-5.2.0-py2.7.egg/pbr/tests/testpackage/doc/source/conf.py:25:5: E265 block comment should start with '# '
./.eggs/pbr-5.2.0-py2.7.egg/pbr/tests/testpackage/doc/source/conf.py:74:1: E265 block comment should start with '# '
ERROR: InvocationError for command /home/travis/build/pycontribs/jenkinsapi/.tox/py27/bin/python -m pycodestyle (exited with code 1)
lechat commented 5 years ago

I'd rather not accept this as this will is not backwards compatible and will break code that uses this library.

JesperMonsted commented 5 years ago

I have now placed the new argument last, this should make the method backwards compatible. Unless I have missed something, in which case I'd like to be enlightened!

JesperMonsted commented 5 years ago

Please review merge request.

lechat commented 5 years ago

I think @casz comments in code are valid and make code simpler. Please fix.

Thank you!

JesperMonsted commented 5 years ago

I'm not sure what you want me to fix, As I read the comments by @casz , they are supporting the backwards compatibility already present in the pull-request. After further investigation into the workings of GitHub, I can see that @casz actually approved the changes!

codecov[bot] commented 5 years ago

Codecov Report

Merging #704 into master will increase coverage by <.01%. The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master     #704      +/-   ##
==========================================
+ Coverage   79.06%   79.06%   +<.01%     
==========================================
  Files          34       34              
  Lines        2703     2709       +6     
==========================================
+ Hits         2137     2142       +5     
- Misses        566      567       +1
JesperMonsted commented 5 years ago

Any review progress?

jetersen commented 5 years ago

@J02M my code comments were used to clarify the code behavior, the code behaves exactly as described in my comments. Hence my approval.