Cleanup #118
Cleanup #118
Conversation
dwillmer
commented
Mar 31, 2016
dwillmer
commented
- Consistent use of single/double quotes. Standalone strings were mostly single-quoted, so i stuck with that, whereas docstrings were all double-quoted, so I left them alone.
- Consistent use of ipython copyright header.
- Re-jig any code over 79 chars, except regex lines.
- Remove triple-quoted strings where the string fits on one line.
- Assign repeated code fragments to local variables, where it makes sense.
- Fix typos.
- Consistent spacing around binary operators.
- Remove mutable default arguments.
- Remove whitespace at end-of-line.
- Factor out complex (and long) if-criteria in pickleutil.
- Move global var in test module to function local.
- Consistent UTF-8 encoding notice in each file (required to keep py2.7 tests passing)
|
Thanks for taking the time to work on this. However, we don't generally accept big 'cleanup' PRs - it's liable to cause merge conflicts, it makes git blame less useful, and code style just isn't all that important to us. It's fine to clean up a specific piece of code you're making functional changes to, but our policy is to reject PRs that just do cleanup, especially when they touch many files at once. Sorry this wasn't clear before you started. |
|
I think a blanket rejection is too harsh. This PR does more than simple white space cleanup. And some of that is important refactoring. I agree that the broad code formatting stuff should go in, but the refactoring stuff should. |
|
Oops typo - I agree with Thomas that the simple code formatting stuff - white space quotes. - should not go in |
| allow_none=True) | ||
| # the client class for KM.client() shortcut | ||
| client_class = DottedObjectName('ipykernel.inprocess.BlockingInProcessKernelClient') | ||
| name = 'ipykernel.inprocess.BlockingInProcessKernelClient' |
There was a problem hiding this comment.
It doesn't seem like this should be a name attribute on the kernel manager class.
|
Fair enough, I didn't notice that were actual significant changes amongst all the code style stuff. Scanning through the changes, though, I suspect it's going to be quicker to redo those changes starting from master than to separate them out of the formatting changes in this PR. |
I seem to agree, and i have a hard time finding the actual changes, even with |
Ok, I'm just spend 30 minutes going through that chuck by chunk, I honestly haven't found the parts that is "important refactoring" and will be needed later. I would be happy to get some pointer to where it is. |
4 participants
