Skip to content
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.

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

Fix string concatenation to os.path.join and add exception case #11291

Merged
merged 1 commit into from Dec 30, 2017

Conversation

@dongsam
Copy link
Contributor

commented Sep 8, 2017

Solved some warnings for Python PEP 8 convention

and added verification logic about referenced before assignment for output_type

@fanquake fanquake added the Tests label Sep 8, 2017

test/util/bitcoin-util-test.py Outdated
"""Runs a single test, comparing output and RC to expected output and RC.

Raises an error if input can't be read, executable fails, or output/RC
are not as expected. Error is caught by bctester() and reported.
"""
# Get the exec names and arguments
execprog = buildenv["BUILDDIR"] + "/src/" + testObj['exec'] + buildenv["EXEEXT"]
execargs = testObj['args']
execprog = buildenv["BUILDDIR"] + "/src/" + test_obj['exec'] + buildenv["EXEEXT"]

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 9, 2017

Member

I'd assume those should probably use os.path.join to make it work on other os as well.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 9, 2017

Thank you for putting so much effort in your first contribution. However, according to our developer guidelines we can not accept pull requests that solely change formatting and style.

Nonetheless, you are welcome to address my issue with this file instead. Also note that you can change testObj to test_obj, as formatting fixes are allowed when none-style but refactoring changes are made to the affected lines.

@dongsam

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2017

 @MarcoFalke Thank you for your advice, I just added commit for use os.path.join instead string concatenation

test/util/bitcoin-util-test.py Outdated
"""Runs a single test, comparing output and RC to expected output and RC.

Raises an error if input can't be read, executable fails, or output/RC
are not as expected. Error is caught by bctester() and reported.
"""
# Get the exec names and arguments
execprog = buildenv["BUILDDIR"] + "/src/" + testObj['exec'] + buildenv["EXEEXT"]
execargs = testObj['args']
execprog = os.path.join(buildenv["BUILDDIR"], "src", test_obj["exec"], buildenv["EXEEXT"])

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 10, 2017

Member

The last concatenation is string concatenation.

You can check if the tests pass locally by make check

This comment has been minimized.

Copy link
@dongsam

dongsam Sep 10, 2017

Author Contributor

Thanks I just fixed it, Sorry for my careless mistake

@jnewbery
Copy link
Member

left a comment

utACK. A couple of nits inline and commits should be squashed to a single commit before merge.

test/util/bitcoin-util-test.py Outdated

def bctester(testDir, input_basename, buildenv):
def bctester(test_dir, input_basename, buildenv):

This comment has been minimized.

Copy link
@jnewbery

jnewbery Sep 12, 2017

Member

since you're converting to snake_case, how about build_env

test/util/bitcoin-util-test.py Outdated
"""Runs a single test, comparing output and RC to expected output and RC.

Raises an error if input can't be read, executable fails, or output/RC
are not as expected. Error is caught by bctester() and reported.
"""
# Get the exec names and arguments
execprog = buildenv["BUILDDIR"] + "/src/" + testObj['exec'] + buildenv["EXEEXT"]
execargs = testObj['args']
execprog = os.path.join(buildenv["BUILDDIR"], "src", test_obj["exec"]) + buildenv["EXEEXT"]

This comment has been minimized.

Copy link
@jnewbery

jnewbery Sep 12, 2017

Member

exec_prog? (and exec_args and exec_run)

@dongsam dongsam force-pushed the dongsam:Fix-PEP8-warnings branch Sep 12, 2017

@MarcoFalke
Copy link
Member

left a comment

Again, I appreciate the effort, but according to the developer notes such refactoring patches can not be accepted.

Do not submit patches solely to modify the style of existing code.

And https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring

This will cause merge conflicts with other open pull requests and consumes precious review time.

Please note that the non-refactoring changes in this pull are much appreciated and I'd like to have them merged cleanly without the formatting changes.

test/util/bitcoin-util-test.py Outdated

parser = argparse.ArgumentParser(description=__doc__)
parser.add_argument('-v', '--verbose', action='store_true')
parser.add_argument("-v", "--verbose", action="store_true")

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 12, 2017

Member

I don't it is worth the overhead to change apostrophe to quotation mark.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Sep 12, 2017

Member

agree (and for short symbol-like strings as in this line, single quotes seem to be most people's preferred style)

test/util/bitcoin-util-test.py Outdated
# Add the format/level to the logger
logging.basicConfig(format=formatter, level=level)

bctester(config["environment"]["SRCDIR"] + "/test/util/data", "bitcoin-util-test.json", config["environment"])
bctester(os.path.join(config["environment"]["SRCDIR"], "test/util/data"), "bitcoin-util-test.json",
config["environment"])

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 12, 2017

Member

Changing this line solely to split it into two causes difficulties grepping in the future.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Sep 12, 2017

Member

... and most of our python files don't adhere to the 79 char max line length (which I think is a good thing!)

test/util/bitcoin-util-test.py Outdated
logging.info("PASSED: " + test_obj["description"])
except Exception as e:
logging.info("FAILED: " + test_obj["description"])
logging.error("Error %s: %s" % (test_obj["description"], e))

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 12, 2017

Member

Can you explain why it makes sense to first log to info and then the same to the error level?

test/util/bitcoin-util-test.py Outdated
if not output_data:
logging.error("Output data missing for " + output_fn)
raise Exception
if not output_type:

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 12, 2017

Member

Generally, I'd prefer if refactoring is separate (at least a separate commit) from changing behavior.

test/util/bitcoin-util-test.py Outdated
if "error_txt" in testObj:
want_error = testObj["error_txt"]
if "error_txt" in test_obj:
want_error = test_obj["error_txt"]

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 12, 2017

Member

I don't think it makes sense to replace every variable in the whole file by a lower_case name.

test/util/bitcoin-util-test.py Outdated
@@ -20,40 +20,44 @@
import subprocess
import sys


This comment has been minimized.

Copy link
@jnewbery

jnewbery Sep 12, 2017

Member

For short util modules like this, and even for most of our test scripts, I don't think 2 lines separating each function is necessary. It makes sense for large projects where you want visual separation between top-level classes, but I think it's unnecessary here.

@jnewbery

This comment has been minimized.

Copy link
Member

commented Sep 12, 2017

@dongsam we don't have particularly strict style guidelines for our python code. The only style notes are in https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md.

Since A Foolish Consistency is the Hobgoblin of Little Minds, we don't usually open PRs to adhere to PEP-8 style, unless there's some good reason to do so (eg making bugs less likely). If I'm doing substantial work on a test script, I often start the PR with a tidy-up commit, but even that is sometimes unpopular and puts off reviewers.

If you're looking for your first commit, I think @MarcoFalke is right that removing the style changes from this PR and just having the os.path changes would make this more palatable for reviewers.

@dongsam dongsam force-pushed the dongsam:Fix-PEP8-warnings branch 3 times, most recently Sep 13, 2017

@dongsam dongsam changed the title Fix PEP8 warnings about formatting Fix string concatenation to os.path.join and add exception case Sep 13, 2017

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Oct 5, 2017

Needs rebase

@dongsam dongsam force-pushed the dongsam:Fix-PEP8-warnings branch Oct 5, 2017

test/util/bitcoin-util-test.py Outdated
@@ -43,12 +43,11 @@ def main():
formatter = '%(asctime)s - %(levelname)s - %(message)s'
# Add the format/level to the logger
logging.basicConfig(format=formatter, level=level)

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 5, 2017

Member

nit: why remove this line? Blank lines are used within functions to indicate logical sections.

This comment has been minimized.

Copy link
@dongsam

dongsam Oct 5, 2017

Author Contributor

sorry, that was mistake while rebasing, I'll add again now

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 5, 2017

Member

You've now added trailing whitespace :(

@dongsam dongsam force-pushed the dongsam:Fix-PEP8-warnings branch Oct 5, 2017

test/util/bitcoin-util-test.py Outdated
@@ -43,12 +43,11 @@ def main():
formatter = '%(asctime)s - %(levelname)s - %(message)s'
# Add the format/level to the logger
logging.basicConfig(format=formatter, level=level)

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 5, 2017

Member

You've now added trailing whitespace :(

inputData = open(filename).read()
stdinCfg = subprocess.PIPE

# Read the expected output data (if there is any)
outputFn = None
outputData = None
outputType = None

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 5, 2017

Member

Is this change (and the one at line 107) intentional?

This comment has been minimized.

Copy link
@dongsam

dongsam Oct 5, 2017

Author Contributor

Yes, there was potential risk outputType can be None so I added initial outputType and exception line when outputType is None, if you think it is not needed, i can remove again

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 9, 2017

Member

Yes, this is a good additional check.

@dongsam dongsam force-pushed the dongsam:Fix-PEP8-warnings branch Oct 5, 2017

test/util/bitcoin-util-test.py Outdated
except:
logging.error("Output file " + outputFn + " can not be opened")
raise
if not outputData:
logging.error("Output data missing for " + outputFn)
raise Exception
if not outputType:
logging.error("Output type missing for " + outputFn)

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 9, 2017

Member

suggestion: change the error text slightly here to clarify what the problem is:

logging.error("Output file %s does not have a file extension" % outputFn)

This comment has been minimized.

Copy link
@fanquake

fanquake Nov 17, 2017

Member

@dongsam Could you address @jnewbery's nit here? Looks like this is pretty much ready for merge.

This comment has been minimized.

Copy link
@dongsam

dongsam Dec 8, 2017

Author Contributor

@fanquake I just change and rebase the error text, sorry for late response

@dongsam dongsam force-pushed the dongsam:Fix-PEP8-warnings branch Dec 8, 2017

test/util/bitcoin-util-test.py Outdated
@@ -77,32 +77,36 @@ def bctest(testDir, testObj, buildenv):
are not as expected. Error is caught by bctester() and reported.
"""
# Get the exec names and arguments
execprog = buildenv["BUILDDIR"] + "/src/" + testObj['exec'] + buildenv["EXEEXT"]
execprog = os.path.join(buildenv["BUILDDIR"], "src", testObj["exec"]) + buildenv["EXEEXT"]

This comment has been minimized.

Copy link
@jnewbery

jnewbery Dec 8, 2017

Member

I think this is better as:

execprog = os.path.join(buildenv["BUILDDIR"], "src", testObj["exec"] + buildenv["EXEEXT"])

(ie buildenv["EXEEXT"] should be appended to testObj["exec"] before joining into a path), but I don't have a windows machine to test this on.

Is there a reason you put buildenv["EXEEXT"] outside the os.path.join?

This comment has been minimized.

Copy link
@dongsam

dongsam Dec 9, 2017

Author Contributor

I thought also but that was mistook, buildenv["EXEEXT"] is not path, so occur error when with os.path.join
that need just string concatenation

comment

This comment has been minimized.

Copy link
@jnewbery

jnewbery Dec 9, 2017

Member

right, that's why you should have:

execprog = os.path.join(buildenv["BUILDDIR"], "src", testObj["exec"] + buildenv["EXEEXT"])

(concatenate testObj["exec"] and buildenv["EXEEXT"] as a single argument to os.path.join())

This comment has been minimized.

Copy link
@dongsam

dongsam Dec 9, 2017

Author Contributor

oh, sorry I misunderstood, that's look better, I'll update :)

@dongsam dongsam force-pushed the dongsam:Fix-PEP8-warnings branch to a3ac767 Dec 9, 2017

@jnewbery

This comment has been minimized.

Copy link
Member

commented Dec 9, 2017

utACK a3ac767. Needs to be tested on windows.

@@ -48,7 +48,7 @@ def main():

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Dec 9, 2017

Member

^ one line above.

"test/util/data" throws an error on windows:

FileNotFoundError: [Errno 2] No such file or directory: '.../Downloads/workspace/bitcoin\\test/util/data\\bitcoin-util-test.json'
@@ -48,7 +48,7 @@ def main():

def bctester(testDir, input_basename, buildenv):
""" Loads and parses the input file, runs all tests and reports results"""
input_filename = testDir + "/" + input_basename
input_filename = os.path.join(testDir, input_basename)

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Dec 9, 2017

Member

Might as well pass in input_filename to bctester and instead do the path.join in line 47, no?

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Dec 30, 2017

Going to merge since this is a strict improvement.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

utACK a3ac7672ed9c63b954d3f368a90616448d31b4c3
-----BEGIN PGP SIGNATURE-----

iQIcBAEBCgAGBQJaRyMzAAoJENLqSFDnUoslIfgP/0jDVYpEb4TBa5/gBHwQJWqk
ez+gDdtwFpJteURQoc4sDFzkGq4xFiltUBuZoDspjgOq8eETvn7mgGpd4t71ZwUy
/7t8yIbFFVR94RhOXCRN/8z9u4vfE7068CeiNwmq+9tP6fRGpsfqCKSbY4qaYuR5
c0RtmOds3ZEknAASMcarT0b+iiBv9gN3ryVlRochrWiO92/lRpfWYvkv0HldDZba
+N1SAPju8KfSjcZRvZ6/3vOyfYChgrXh+y67MJLMGZCkct8YllcjXvPYy5Km74wI
oEZm/TBq4aW3FNgDk4EPHWnKVCA/qC73TzaO3V4QnfqEZz5pXxSEbhldQpo7q+Lm
s/Ve9FJdwMtnS1Gk+wrzoFZCioq6DVgoWixmwT0eJYB2bL9IeeBinceAIOAr08Sz
3IgXgBgaxkaLZSYBp8kkkIsn8WYi0JD2tnDVxcTSBaGH1igdKogLWQDXqEd+Mu1D
k+EArOVMWWeX/pgGoEV1u8Ze7mvZSGwxJxHsJtWGdCoXPgrFrZQFpaba4JkrGTq8
d1vg+8v1+kpxSGlpehiX+2sEunrU5c6XwmSny2zAZassBJ0sTySglhwlqmI493fP
3d0h0mOI2OX7SMxfos1psIrAOTQEEikWqMsxLnG/hehAlj7vOit7GFYCRha6kduE
ElsosotNEXWBPPI7crsO
=hB1L
-----END PGP SIGNATURE-----

@MarcoFalke MarcoFalke merged commit a3ac767 into bitcoin:master Dec 30, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Dec 30, 2017

Merge #11291: Fix string concatenation to os.path.join and add except…
…ion case

a3ac767 Fix string concatenation to os.path.join and add exception case (dongsamb)

Pull request description:

  Solved some warnings for [Python PEP 8 convention](https://www.python.org/dev/peps/pep-0008/)

  - [Method Names and Instance Variables](https://www.python.org/dev/peps/pep-0008/#method-names-and-instance-variables)
  lowercase with words separated by underscores as necessary to improve readability.

      - `testDir` to `test_dir`
      - `inputData` to `input_data`
      - ...

  - [Blank Lines](https://www.python.org/dev/peps/pep-0008/#blank-lines)
  Surround top-level function and class definitions with two blank lines.

  - [Exception Names](https://www.python.org/dev/peps/pep-0008/#exception-names)

  and added verification logic about referenced before assignment for `output_type`

Tree-SHA512: 346d08799f03077a2b7257ccdca123b4945b89dbf0677dba452d96b81ce186ec7b5dcdb10b8bb59cfce657a7aedbb7df64921036cbd1bf4ad8bd313d40faa796
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.