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

Code parsing for run selection in terminal - Python side #14457

Merged
merged 20 commits into from Oct 23, 2020

Conversation

kimadeline
Copy link

@kimadeline kimadeline commented Oct 20, 2020

For #14048

If I may suggest, it might be easier if you checked out the PR locally or looked at the new files directly instead of doing side-by-side comparisons, because it's, uh, not pretty.

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • Has a news entry file (remember to thank yourself!).
  • Appropriate comments and documentation strings in the code.
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated.
  • Test plan is updated as appropriate.
  • package-lock.json has been regenerated by running npm install (if dependencies have changed).
  • The wiki is updated with any design decisions/details.

@codecov-io
Copy link

codecov-io commented Oct 20, 2020

Codecov Report

Merging #14457 into main will increase coverage by 0.10%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #14457      +/-   ##
==========================================
+ Coverage   59.18%   59.28%   +0.10%     
==========================================
  Files         720      720              
  Lines       40216    40332     +116     
  Branches     5830     5849      +19     
==========================================
+ Hits        23803    23912     +109     
- Misses      15151    15159       +8     
+ Partials     1262     1261       -1     
Impacted Files Coverage Δ
...pythonEnvironments/common/environmentIdentifier.ts 96.77% <0.00%> (-0.20%) ⬇️
src/client/datascience/constants.ts 99.78% <0.00%> (-0.01%) ⬇️
src/client/telemetry/constants.ts 100.00% <0.00%> (ø)
src/client/common/serviceRegistry.ts 100.00% <0.00%> (ø)
...ience/interactive-common/interactiveWindowTypes.ts 100.00% <0.00%> (ø)
...very/locators/services/virtualenvwrapperLocator.ts
...ythonEnvironments/common/virtualenvwrapperUtils.ts
...s/discovery/locators/services/virtualenvLocator.ts
...onments/discovery/locators/services/venvLocator.ts
...ocators/services/globalVirtualEnvronmentLocator.ts 100.00% <0.00%> (ø)
... and 4 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 25fd710...394af97. Read the comment docs.

@kimadeline kimadeline marked this pull request as ready for review October 21, 2020 20:00
@kimadeline
Copy link
Author

kimadeline commented Oct 21, 2020

Just to be sure, I also ran single-workspace tests with the changes that Karthik introduces in #14445, and none of the failures were caused by this code.

I also fixed whatever single and multi-workspace test suites that broke from this change (or at least, that the failures weren't caused by this code):

https://dev.azure.com/ms/vscode-python/_build/results?buildId=117067&view=results

image

(Test failure reported in #14469)

# >>> total = x + y
# >>>
if end - start > 1:
block += "\n"
Copy link

Choose a reason for hiding this comment

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

One thing that didn't occur to me when we were discussing this, is that for the multiline parentheses case, this newline is redundant, since the closing parenthesis terminates the statement already. So for this input:

x = [
   1
]
y = [
   2
]

The REPL will get a blank line between the two, even though it's not strictly needed:

>>> x = [
...   1
... ]
>>> 
>>> y = [
...   2
...]

I think that's okay, since that extra blank line doesn't change behavior, and appearance is not such a big deal for an uncommon code pattern. But it might be worth pointing out in the comments.

Copy link
Member

Choose a reason for hiding this comment

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

Does it change the repel default _ by any chance? It should not, but just curious.

Copy link
Author

Choose a reason for hiding this comment

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

But it might be worth pointing out in the comments.

I'll add a comment.

Does it change the repel default _ by any chance?

You mean the extra blank statement? Doesn't seem like it:

image

pythonFiles/normalizeForInterpreter.py Outdated Show resolved Hide resolved
@karthiknadig
Copy link
Member

Two more changes:

  1. A send the text as is if parsing fails
  2. Put this change in a separate python file, so we can use A/B testing.

Copy link
Member

@karthiknadig karthiknadig left a comment

Choose a reason for hiding this comment

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

See previous comment

@kimadeline
Copy link
Author

kimadeline commented Oct 23, 2020

send the text as is if parsing fails

Do we want this in this PR despite being a separate work item? Did it

Put this change in a separate python file, so we can use A/B testing.

👍

@sonarcloud
Copy link

sonarcloud bot commented Oct 23, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@kimadeline kimadeline merged commit a70c2eb into microsoft:main Oct 23, 2020
@kimadeline kimadeline deleted the 14048-run-selection-python-side branch October 23, 2020 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants