[Python] Improve Python consistency: Use function_name(…) throughout (PEP8) #1399

practicalswift · 2016-02-22T22:02:47Z

Background: See discussion with @modocache and @nadavrot in #1369. This is a follow-up to #1374.

The majority of Python function names in the repo are in lowercase with words separated by underscores (snake_case_function). This is also the form recommended by the PEP8 function naming recommendations (Python Software Foundation).

This PR improves consistency by converting the remaning cases of camelCaseFunction to the PEP-8 form (snake_case_function) used by convention in the repo.

@modocache - do you have time to review? That would be really appreciated :-)

gribozavr · 2016-02-22T22:06:04Z

@swift-ci Please test

modocache · 2016-02-22T22:11:06Z

Awesome! What's left before we can activate the .pep8 rule for method/variable names?

modocache · 2016-02-22T22:12:10Z

tools/SourceKit/bindings/python/sourcekitd/__init__.py

@@ -21,4 +21,4 @@
    Bindings for the sourcekitd framework.
 """

-__all__ = ['capi', 'request']
+__all__ = ('capi', 'request')


All of these transformations to __all__ are probably incorrect, no? As far as I know, __all__ is a list, not a tuple.

Did your search and replace misfire?

modocache · 2016-02-22T22:14:52Z

This looks great, aside from the changes to __all__--the documentation indicates that it should be a list: https://docs.python.org/2/tutorial/modules.html#importing-from-a-package. I have a feeling the changes to __all__ will cause CI to fail here.

practicalswift · 2016-02-22T23:03:39Z

@modocache Thanks for reviewing! :-)

The pydocstyle/pep257 tool requires __all__ to be a tuple. See discussion here: PyCQA/pydocstyle#62

When thinking of it the __all__ change has nothing to do with function naming, so I've reverted that change in 23abb10e8584e4f88d6008dd40c7aade6b8a4da0 which is now part of this PR.

Thanks for noticing!

practicalswift · 2016-02-26T23:39:24Z

@gribozavr Merge conflicts have now been resolved :-)

practicalswift · 2016-02-28T16:26:54Z

@gribozavr + @nadavrot Another round of newly introduced merge conflicts resolved :-)

Let me know if any changes are needed for this PR to be merged.

practicalswift · 2016-02-28T16:29:01Z

@modocache Regarding your question about the .pep8 rule for method/variable names - do you mean the rules in pep8-naming? :-)

gottesmm · 2016-02-28T21:47:07Z

@swift-ci Please test

practicalswift · 2016-02-29T08:07:41Z

@gottesmm Both builds passed. Are there any further changes needed for this PR to be merged? :-)

gottesmm · 2016-02-29T08:59:32Z

@practicalswift Out of curiosity, how did you generate the changes? Were you using something like rope?

gottesmm · 2016-02-29T19:50:36Z

@practicalswift The reason why I am asking is I want to know if I should lint the code while reviewing since some of this code is not invoked by the normal build.

practicalswift · 2016-02-29T21:37:16Z

@gottesmm All changes were made manually using query-replace in Emacs :-) However, I used flake8 to identify the issues.

I also used git grep … extensively to double-check that all references were covered by the renames.

…(PEP8)

practicalswift · 2016-02-29T21:50:26Z

Another newly introduced merge conflict now resolved :-)

practicalswift · 2016-02-29T21:57:58Z

@gottesmm This oneliner can be used to list the old names of the renamed functions:

$ git log -u -n 1 | egrep '^-.*def ' | cut -b2- | sed 's/^ *//g' | cut -f1 -d'(' | awk '{ print $2 }' | sort | uniq
accumulateCode
addFunction
addLine
addPass
appendText
bodyLines
codeStartsWithDedentKeyword
compareFunctionSizes
compareScores
compareSizes
compareSizesOfFile
compileOpaqueCFile
compileSource
compileSources
computeItersNumber
executeTemplate
formatChildren
get_BMP_data_offset
get_BMP_first_level_index
get_grapheme_cluster_break_tests_as_UTF8
getLineStarts
getScores
getSuccs
_int_list_to_LE_bytes
_int_to_LE_bytes
isMaxScore
listFunctionSizes
nextToken
numeric_type_names_Macintosh_only
parseArguments
parseBenchmarkOutput
parseGenericOperator
parseInt
parseProtocol
parseTemplate
posToLine
Print
printBestScores
printCommand
Process
processSource
processSources
readSizes
reportResults
runBench
runBenchmarks
runCommand
splitGybLines
splitLines
stripTrailingNL
submit_to_LNT
tokenGenerator
tokenizePythonToUnmatchedCloseCurly
tokenizeTemplate
tokenPosToIndex

practicalswift · 2016-02-29T22:04:44Z

And this to search for occurrences of the old names of the functions to make sure there are no remaining references to them in the repo:

$ for OLD_NAME in $(git log -u -n 1 | egrep '^-.*def ' | cut -b2- | sed 's/^ *//g' | cut -f1 -d'(' | awk '{ print $2 }' | sort | uniq); do echo "[search for: '${OLD_NAME}']"; git grep -e "${OLD_NAME}[^a-zA-Z]" | egrep -v '\.(cpp|h|swift|sil|md|txt|json|mm|td|def):'; done
[search for: 'accumulateCode']
[search for: 'addFunction']
[search for: 'addLine']
[search for: 'addPass']
[search for: 'appendText']
[search for: 'bodyLines']
[search for: 'codeStartsWithDedentKeyword']
[search for: 'compareFunctionSizes']
[search for: 'compareScores']
[search for: 'compareSizes']
[search for: 'compareSizesOfFile']
[search for: 'compileOpaqueCFile']
[search for: 'compileSource']
[search for: 'compileSources']
[search for: 'computeItersNumber']
[search for: 'executeTemplate']
[search for: 'formatChildren']
[search for: 'get_BMP_data_offset']
[search for: 'get_BMP_first_level_index']
[search for: 'get_grapheme_cluster_break_tests_as_UTF8']
[search for: 'getLineStarts']
[search for: 'getScores']
[search for: 'getSuccs']
[search for: '_int_list_to_LE_bytes']
[search for: '_int_to_LE_bytes']
[search for: 'isMaxScore']
[search for: 'listFunctionSizes']
[search for: 'nextToken']
[search for: 'numeric_type_names_Macintosh_only']
[search for: 'parseArguments']
[search for: 'parseBenchmarkOutput']
[search for: 'parseGenericOperator']
[search for: 'parseInt']
[search for: 'parseProtocol']
[search for: 'parseTemplate']
[search for: 'posToLine']
[search for: 'Print']
stdlib/public/core/Arrays.swift.gyb:      debugPrint(item, terminator: "", toStream: &result)
stdlib/public/core/HashedCollections.swift.gyb:      debugPrint(member, terminator: "", toStream: &result)
stdlib/public/core/HashedCollections.swift.gyb:      debugPrint(k, terminator: "", toStream: &result)
stdlib/public/core/HashedCollections.swift.gyb:      debugPrint(v, terminator: "", toStream: &result)
utils/build-script-impl:# Print instructions for using this script to stdout
utils/error_enum_to_cases.pl:# Print properties for the ranges.
utils/find-unused-diagnostics.sh:# Print all diags that occur in the .td files but not in the source.
utils/swift-api-dump.py:    parser.add_argument('-v', '--verbose', action='store_true', help='Print extra information.')
utils/swift-api-dump.py:# Print out the command we're about to execute
utils/swift_build_support/swift_build_support/debug.py:# swift_build_support/debug.py - Print information on the build -*- python -*-
utils/swift_build_support/swift_build_support/debug.py: Print the host machine's `xcodebuild` version, as well as version
[search for: 'printBestScores']
[search for: 'printCommand']
[search for: 'Process']
benchmark/scripts/Benchmark_Driver:            print "Process output:\n", e.output
benchmark/scripts/Benchmark_RuntimeLeaksRunner.in:            print("Child Process Failed! (%s,%s)" % (data['path'], data['test_name']))
cmake/modules/AddSwift.cmake:# Process the sources within the given variable, pulling out any Swift
stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb:  print("$ \(Process.arguments[0]) " +
stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb:func _childProcess() {
stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb:struct _ParentProcess {
stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb:  internal var _runTestsInProcess: Bool
stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb:  init(runTestsInProcess: Bool, args: [String], filter: String?) {
stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb:    self._runTestsInProcess = runTestsInProcess
stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb:        if _runTestsInProcess {
stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb:  let _isChildProcess: Bool =
stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb:    Process.arguments.contains("--stdlib-unittest-run-child")
stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb:  if _isChildProcess {
stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb:    _childProcess()
stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb:    var runTestsInProcess: Bool = false
stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb:    while i < Process.arguments.count {
stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb:      let arg = Process.arguments[i]
stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb:        runTestsInProcess = true
stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb:        filter = Process.arguments[i + 1]
stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb:      args.append(Process.arguments[i])
stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb:    var parent = _ParentProcess(
stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb:      runTestsInProcess: runTestsInProcess, args: args, filter: filter)
utils/profdata_merge/process.py:class ProfdataMergerProcess(Process):
utils/profdata_merge/process.py:        super(ProfdataMergerProcess, self).__init__()
utils/profdata_merge/runner.py:    processes = [ProfdataMergerProcess(config, file_queue) for _ in range(10)]
utils/profdata_merge/runner.py:    merge_final = ProfdataMergerProcess(config, file_queue)
utils/swift-bench.py:  if Process.arguments.count > 1 {
utils/swift-bench.py:    N = Process.arguments[1].toInt()!
utils/swift-bench.py:  if Process.arguments.count <= 2 || Process.arguments[2] == name {
[search for: 'processSource']
[search for: 'processSources']
[search for: 'readSizes']
[search for: 'reportResults']
[search for: 'runBench']
[search for: 'runBenchmarks']
[search for: 'runCommand']
[search for: 'splitGybLines']
[search for: 'splitLines']
[search for: 'stripTrailingNL']
[search for: 'submit_to_LNT']
[search for: 'tokenGenerator']
[search for: 'tokenizePythonToUnmatchedCloseCurly']
[search for: 'tokenizeTemplate']
[search for: 'tokenPosToIndex']

gottesmm · 2016-02-29T22:25:11Z

Ok. I am going to just run it through pylint and then I will merge assuming nothing comes up.

practicalswift · 2016-02-29T22:31:34Z

@gottesmm Excellent! 👍 Thanks!

gottesmm · 2016-02-29T22:52:15Z

pylint -E was not clean, but none of it was due to this commit. This commit LGTM.

[Python] Improve Python consistency: Use function_name(…) throughout (PEP8)

Use fatal_error instead of diagnostics in build-script

modocache reviewed Feb 22, 2016
View reviewed changes

practicalswift mentioned this pull request Feb 23, 2016

[Python] Update .pep8 with ignored docstring rules #1402

Merged

[Python] Improve Python consistency: Use function_name(…) throughout …

f6d6585

…(PEP8)

gottesmm added a commit that referenced this pull request Feb 29, 2016

Merge pull request #1399 from practicalswift/idiomatic-python-part-ii

e7738bb

[Python] Improve Python consistency: Use function_name(…) throughout (PEP8)

gottesmm merged commit e7738bb into swiftlang:master Feb 29, 2016

practicalswift mentioned this pull request Mar 2, 2016

[Python] Improve Python consistency: "variable in function should be lowercase" (N806) #1512

Merged

1 task

MaxDesiatov added a commit that referenced this pull request Oct 19, 2020

Merge pull request #1399 from swiftwasm/maxd/cleanup-build-script

0211512

Use fatal_error instead of diagnostics in build-script

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Python] Improve Python consistency: Use function_name(…) throughout (PEP8) #1399

[Python] Improve Python consistency: Use function_name(…) throughout (PEP8) #1399

practicalswift commented Feb 22, 2016

gribozavr commented Feb 22, 2016

modocache commented Feb 22, 2016

modocache Feb 22, 2016

modocache Feb 22, 2016

modocache commented Feb 22, 2016

practicalswift commented Feb 22, 2016

practicalswift commented Feb 26, 2016

practicalswift commented Feb 28, 2016

practicalswift commented Feb 28, 2016

gottesmm commented Feb 28, 2016

practicalswift commented Feb 29, 2016

gottesmm commented Feb 29, 2016

gottesmm commented Feb 29, 2016

practicalswift commented Feb 29, 2016

practicalswift commented Feb 29, 2016

practicalswift commented Feb 29, 2016

practicalswift commented Feb 29, 2016

gottesmm commented Feb 29, 2016

practicalswift commented Feb 29, 2016

gottesmm commented Feb 29, 2016

[Python] Improve Python consistency: Use function_name(…) throughout (PEP8) #1399

[Python] Improve Python consistency: Use function_name(…) throughout (PEP8) #1399

Conversation

practicalswift commented Feb 22, 2016

gribozavr commented Feb 22, 2016

modocache commented Feb 22, 2016

modocache Feb 22, 2016

Choose a reason for hiding this comment

modocache Feb 22, 2016

Choose a reason for hiding this comment

modocache commented Feb 22, 2016

practicalswift commented Feb 22, 2016

practicalswift commented Feb 26, 2016

practicalswift commented Feb 28, 2016

practicalswift commented Feb 28, 2016

gottesmm commented Feb 28, 2016

practicalswift commented Feb 29, 2016

gottesmm commented Feb 29, 2016

gottesmm commented Feb 29, 2016

practicalswift commented Feb 29, 2016

practicalswift commented Feb 29, 2016

practicalswift commented Feb 29, 2016

practicalswift commented Feb 29, 2016

gottesmm commented Feb 29, 2016

practicalswift commented Feb 29, 2016

gottesmm commented Feb 29, 2016