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

Enhancements to running code in a terminal #1432

Merged
merged 12 commits into from
Apr 23, 2018

Conversation

DonJayamanne
Copy link

@DonJayamanne DonJayamanne commented Apr 18, 2018

Fixes #1207
Fixes #1316
Fixes #1349
Fixes #259

This pull request:

  • Has a title summarizes what is changing
  • Includes a news entry file (remember to thank yourself!)
  • Has unit tests & code coverage is not adversely affected (within reason)
  • Works on all actively maintained versions of Python (e.g. Python 2.7 & the latest Python 3 release)
  • Works on Windows 10, macOS, and Linux (e.g. considered file system case-sensitivity)

@DonJayamanne DonJayamanne changed the title Enhancements to running code in a terminal WIP - Enhancements to running code in a terminal Apr 19, 2018
@DonJayamanne DonJayamanne changed the title WIP - Enhancements to running code in a terminal Enhancements to running code in a terminal Apr 19, 2018
@codecov
Copy link

codecov bot commented Apr 20, 2018

Codecov Report

Merging #1432 into master will decrease coverage by 25.51%.
The diff coverage is 75%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1432       +/-   ##
===========================================
- Coverage    70.8%   45.29%   -25.52%     
===========================================
  Files         271      271               
  Lines       12519    12550       +31     
  Branches     2224     2231        +7     
===========================================
- Hits         8864     5684     -3180     
- Misses       3512     6716     +3204     
- Partials      143      150        +7
Impacted Files Coverage Δ
src/client/common/configSettings.ts 87.57% <ø> (-0.6%) ⬇️
src/client/terminals/types.ts 100% <ø> (ø) ⬆️
src/client/common/types.ts 100% <ø> (ø) ⬆️
...erminals/codeExecution/djangoShellCodeExecution.ts 40% <0%> (-60%) ⬇️
...nt/terminals/codeExecution/codeExecutionManager.ts 48.83% <0%> (-51.17%) ⬇️
...t/terminals/codeExecution/terminalCodeExecution.ts 15.38% <33.33%> (-84.62%) ⬇️
src/client/terminals/codeExecution/helper.ts 92.85% <89.65%> (-4.29%) ⬇️
src/client/language/tokenizer.ts 4.4% <0%> (-92.4%) ⬇️
src/client/formatters/lineFormatter.ts 7.75% <0%> (-91.48%) ⬇️
src/client/language/characterStream.ts 4.41% <0%> (-91.18%) ⬇️
... and 135 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 759bbce...578c359. Read the comment docs.

@DonJayamanne
Copy link
Author

DonJayamanne commented Apr 20, 2018

@brettcannon Please review.
I think I've managed to resolve most of the terminal issues (except for the add new line to execute).
This resolves issues where we remove blank lines when when shouldn't and vice versa.

See #259 for details.

Oh yes, this will require a lot of testing.

Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

I would also add a test that includes UTF-8 Unicode characters.

unicode = str


def normalizeLines(content):
Copy link
Member

Choose a reason for hiding this comment

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

normalize_lines

Copy link
Author

Choose a reason for hiding this comment

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

aaarg..



def normalizeLines(content):
"""Removes empty lines and adds empty lines only to sepaparate
Copy link
Member

Choose a reason for hiding this comment

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

Docstrings always have a single line summary that fits within the 80 column limit, then a newline, then anything extra. E.g.

def normalize_lines(content):
    """Normalize blank lines for sending to the terminal.

    Blank lines within a statement block are removed to prevent the REPL
    from thinking the block is finished. Newlines are added to separate
    top-level statements so that the REPL does not think there is a syntax
    error.

    """
    lines = content.splitlines(False)
    # ...


lines = content.splitlines(False)

# Find out if we have any trailing blank lines
Copy link
Member

Choose a reason for hiding this comment

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

Missing a period.

# Find out if we have any trailing blank lines
has_blank_lines = len(lines[-1].strip()) == 0 or content.endswith(os.linesep)

# Remove empty lines
Copy link
Member

Choose a reason for hiding this comment

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

Missing a period.

for line_index in reversed(new_lines_to_remove):
lines.pop(line_index)

# Add new lines just before every dedent
Copy link
Member

Choose a reason for hiding this comment

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

Missing a period.



if __name__ == '__main__':
contents = unicode(sys.argv[1])
Copy link
Member

Choose a reason for hiding this comment

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

What are you trying to accomplish here? This is a rather dramatic decoding if you were handed bytes through sys.argv.

Copy link
Author

Choose a reason for hiding this comment

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

When sending via stdin for python2, bytes get sent in stdin.

has_blank_lines = len(lines[-1].strip()) == 0 or content.endswith(os.linesep)

# Remove empty lines
tokens = tokenize.generate_tokens(io.StringIO(content).readline)
Copy link
Member

Choose a reason for hiding this comment

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

FYI that is undocumented in Python 3. Is there a reason you aren't using tokenize.tokenize()?

@DonJayamanne DonJayamanne merged commit 55f5ca5 into microsoft:master Apr 23, 2018
@DonJayamanne DonJayamanne deleted the issue1207RemoveBlankLines branch June 20, 2018 03:14
@lock lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants