Skip to content

Add indent and autopep8 infrastructure #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Mar 14, 2018
Merged

Add indent and autopep8 infrastructure #2

merged 4 commits into from
Mar 14, 2018

Conversation

tiran
Copy link
Member

@tiran tiran commented Nov 24, 2017

  • indent is a tool to auto-format C code. The .indent.pro file is based on CPython's Misc/indent.pro file. It should get us very close to PEP 7 compatible C code.
  • autopep8 is a Python library to auto-format Python code. It can be used to address single pycodestyle violation, e.g. E121 - Fix indentation to be a multiple of four.. See https://pypi.python.org/pypi/autopep8#features

@encukou encukou mentioned this pull request Nov 24, 2017
Closed
@tiran tiran force-pushed the autoformat branch 3 times, most recently from 76b7aa1 to e002422 Compare November 25, 2017 15:19
@codecov codecov bot deleted a comment from codecov-io Nov 25, 2017
@encukou
Copy link
Member

encukou commented Nov 25, 2017

Thanks.
I don't think installing tools from the Makefile is a good idea, though. Would you be OK limiting that to just running the tool?
The installation could be documented separately (but not necessarily; autopep8: command not found is quite an actionable error message).

@tiran
Copy link
Member Author

tiran commented Nov 26, 2017

Ah, Fedora has python3-autopep8 package. Good, that makes our job a but easier.

@codecov
Copy link

codecov bot commented Nov 27, 2017 •
edited
Loading

Codecov Report

Merging #2 into master will decrease coverage by 4.42%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master       #2      +/-   ##
==========================================
- Coverage   69.51%   65.08%   -4.43%     
==========================================
  Files          49       47       -2     
  Lines        4740     5136     +396     
  Branches      820      988     +168     
==========================================
+ Hits         3295     3343      +48     
- Misses       1088     1432     +344     
- Partials      357      361       +4
Impacted Files Coverage Δ
Lib/ldap/schema/subentry.py 54.61% <50%> (-13.93%) ⬇️
Lib/ldap/constants.py 0% <0%> (-100%) ⬇️
Lib/ldap/async.py 27.92% <0%> (-72.08%) ⬇️
Modules/options.c 34.42% <0%> (-46.07%) ⬇️
Lib/ldap/schema/models.py 49.47% <0%> (-21.6%) ⬇️
Lib/ldap/ldapobject.py 62.28% <0%> (-8.35%) ⬇️
Lib/ldap/functions.py 68.88% <0%> (-6.67%) ⬇️
Lib/ldap/sasl.py 75% <0%> (-5.56%) ⬇️
Modules/LDAPObject.c 63.97% <0%> (-3.72%) ⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3868ea...0473e87. Read the comment docs.

@tiran tiran force-pushed the autoformat branch 3 times, most recently from 5f184cd to 5a5d98f Compare November 28, 2017 15:05
@encukou
Copy link
Member

encukou commented Nov 30, 2017

Let's worry about this after 3.0.

@encukou encukou added on hold and removed on hold labels Dec 4, 2017
@tiran tiran mentioned this pull request Dec 12, 2017
Closed
@jdufresne
Copy link
Member

I think it would be good to land this change before releasing 3.0.0. A new major release is the best time to do large project wide changes. IIUC, this will modify the code but should not have any side effects. Having more idiomatic Python code as well as avoiding inadvertent whitespace changes will help make new contributions easier.

@encukou
Copy link
Member

encukou commented Dec 13, 2017

3.0.0 is mainly about merging contributors from pyldap, which, being a fork that was meant to be merged back to python-ldap, avoided reformatting the code.
Helping make new contributions easier is the next step, but please wait for after the release. (It should be here soon; there's one bug to fix!)

@encukou encukou added this to the 3.1 milestone Dec 13, 2017
@jdufresne
Copy link
Member

Makes sense. Thanks for the response.

@encukou encukou mentioned this pull request Mar 14, 2018
Merged
@encukou
Copy link
Member

encukou commented Mar 14, 2018

Hi,
I've rebased, added an autoformat target to hide the implementation details, and mentioned this in the Contributing section of the docs. Does this look good?

tiran
tiran commented Mar 14, 2018
View reviewed changes
Copy link
Member Author

@tiran tiran left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM,

please rebase the PR on master.

@encukou encukou closed this Mar 14, 2018
@encukou encukou reopened this Mar 14, 2018
@encukou encukou mentioned this pull request Mar 14, 2018
Merged
@encukou encukou merged commit 613b074 into python-ldap:master Mar 14, 2018
@tiran tiran deleted the autoformat branch March 14, 2018 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.1
Development

Successfully merging this pull request may close these issues.
3 participants
@tiran @encukou @jdufresne