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

Remove shellify #184

Merged
merged 15 commits into from
Apr 23, 2018
Merged

Remove shellify #184

merged 15 commits into from
Apr 23, 2018

Conversation

nth10sd
Copy link
Contributor

@nth10sd nth10sd commented Apr 20, 2018

pathlib2 from PyPI (or pathlib via the Python 3.5+ stdlib) and shellescape from PyPI are the new goodness to use. This is because the custom functions getAbsPathForAdjacentFile, normExpUserPath and shellify do not have tests.

This PR starts us off the route for using pathlib2/pathlib in our tests first, and completely removes shellify. Fixes #183.

@nth10sd nth10sd self-assigned this Apr 20, 2018
@nth10sd
Copy link
Contributor Author

nth10sd commented Apr 20, 2018

Due to test failures, I will have to split off pathlib2 additions into a separate PR, so this will just largely be about shellify replacement by shellescape.

@nth10sd nth10sd changed the title Start using pathlib2 and remove shellify Remove shellify Apr 20, 2018
@codecov-io
Copy link

codecov-io commented Apr 20, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@6929634). Click here to learn what that means.
The diff coverage is 23.07%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #184   +/-   ##
=========================================
  Coverage          ?   20.54%           
=========================================
  Files             ?       27           
  Lines             ?     2565           
  Branches          ?        0           
=========================================
  Hits              ?      527           
  Misses            ?     2038           
  Partials          ?        0
Impacted Files Coverage Δ
src/funfuzz/js/js_interesting.py 20.34% <100%> (ø)
src/funfuzz/util/subprocesses.py 11.36% <12.5%> (ø)
src/funfuzz/js/compare_jit.py 16.36% <14.28%> (ø)
src/funfuzz/util/lithium_helpers.py 13.72% <20%> (ø)
src/funfuzz/js/compile_shell.py 17.23% <33.33%> (ø)
src/funfuzz/js/inspect_shell.py 22.33% <50%> (ø)

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 6929634...b80859f. Read the comment docs.

@nth10sd
Copy link
Contributor Author

nth10sd commented Apr 20, 2018

This latest build failure is unrelated. During the process, mozilla-central upgraded its minimium Python requirement to be 3.5+ (see bug 1451065).

All the checks pass at the previous ec0a3a1 changeset here.

@@ -109,7 +110,7 @@ def compareLevel(jsEngine, flags, infilename, logPrefix, options, showDetailedDi
if (r.return_code == 1 or r.return_code == 2) and (anyLineContains(r.out, "[[script] scriptArgs*]") or (
anyLineContains(r.err, "[scriptfile] [scriptarg...]"))):
print("Got usage error from:")
print(" %s" % sps.shellify(command))
print(" %s" % " ".join([quote(x) for x in command]))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't use str.join([list comprehension]), use str.join(generator expression) instead.
List comprehension builds the whole list in-memory and then calls join() on the result. A generator expression works as though each item is yielded to join() as it is evaluated. The syntax is the same, just drop the square brackets.

This occurs on nearly every line changed, so I haven't added comments on them all.

Copy link
Collaborator

@jschwartzentruber jschwartzentruber left a comment

Choose a reason for hiding this comment

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

lgtm

@nth10sd nth10sd merged commit 6378ca3 into master Apr 23, 2018
@nth10sd nth10sd deleted the use-pathlib2-shellescape-more branch April 23, 2018 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants