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.

Sign up for GitHub

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

Jump to bottom

Fix multiline comments with text on first line. #4884

Merged
merged 1 commit into from
Mar 21, 2019
Merged

Fix multiline comments with text on first line. #4884

merged 1 commit into from
Mar 21, 2019

Conversation

rchiodo
Copy link

@rchiodo rchiodo commented Mar 21, 2019

For #4791

  • 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!)
  • 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.

@rchiodo rchiodo self-assigned this Mar 21, 2019
IanMatthewHuff
IanMatthewHuff reviewed Mar 21, 2019
View reviewed changes
src/client/datascience/common.ts
if (insideMultiline) {
if (!isMultiline) {
foundCommentLine(l, pos);
} else {
insideMultiline = false;

// Might end with text too
if (trim.length > 3) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe this is valid syntax to have valid code lines on the same line as a docstring ends

Copy link
Author

Choose a reason for hiding this comment

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

No it's not. The linter flags it. Do you think we should show nothing then?

The problem is we remove all comments when submitting code. So what should we do with a situation like this?

Maybe we shouldn't be removing all comments, but I'd rather not change that now.

Copy link
Author

Choose a reason for hiding this comment

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

I'd be fine with showing nothing. It would just run the comment and strip out this line

IanMatthewHuff
IanMatthewHuff approved these changes Mar 21, 2019
View reviewed changes
Copy link
Member

@IanMatthewHuff IanMatthewHuff left a comment

Choose a reason for hiding this comment

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

:shipit:

@rchiodo rchiodo merged commit fcf990a into master Mar 21, 2019
@rchiodo rchiodo deleted the rchiodo/fix_multline_comments branch March 21, 2019 22:46
rchiodo added a commit that referenced this pull request Mar 21, 2019
@rchiodo
Fix multiline comments with text on first line. (#4884)
f7a4d53
DonJayamanne added a commit that referenced this pull request Mar 29, 2019
@DonJayamanne
Master to merge (#5020)
24b1158 
* Update tpn distro and 3rd party notices

* Pin to beta version of PTVSD (#4836)

Update version of PTVSD

* Add ignore button to the MacInterpreterSelectedAndNoOtherInterpreters… (#4808)

Add ignore button to the MacInterpreterSelectedAndNoOtherInterpretersDiagnostic & MacInterpreterSelectedAndHaveOtherInterpretersDiagnostic diagnostic message to provide the ability to opt out of the warning. (Fixing related issue #4448)

* Generalize test results service (#4813)

* Generalize test results service

* News entry

* Fix gulp errors

* Updated status icons

* Added tests

* fix names for run above and run below code lenses (#4879)

* Fix multiline comments with text on first line. (#4884)

* Cherry-pick 26a7b9c

* cherry pick from master branch

* Revert "Revert "Always use the same jedi environment (#4687)" (#4850)"

This reverts commit 0268ef4.

* Update change log entries

* Bump ptvsd to 4.2.5 (#4904)

* Bump version of PTVSD to 4.2.5
* Changes to capabilities

* Ensure output panel does not steal focus due to ls errors (#4919)

For #4868

<!--
  If an item below does not apply to you, then go ahead and check it off as "done" and strikethrough the text, e.g.:
    - [x] ~Has unit tests & system/integration tests~
-->
- [x] Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
- [x] Title summarizes what is changing
- [x] Has a [news entry](https://github.com/Microsoft/vscode-python/tree/master/news) file (remember to thank yourself!)
- [n/a] Has sufficient logging.
- [n/a] Has telemetry for enhancements.
- [n/a] Unit tests & system/integration tests are added/updated
- [n/a] [Test plan](https://github.com/Microsoft/vscode-python/blob/master/.github/test_plan.md) is updated as appropriate
- [n/a] [`package-lock.json`](https://github.com/Microsoft/vscode-python/blob/master/package-lock.json) has been regenerated by running `npm install` (if dependencies have changed)
- [n/a] The wiki is updated with any design decisions/details.

* Fix change log

* Bump pinned version of Language Server

* Release for march 2018 (#4931)

* New version
* Updated change log

* Same logic for stable and beta (#4964)

* Point release for March 2018 (#4965)

* Capture telemetry when tests are disabled (#4997)

For #4801

* Update change log
DonJayamanne added a commit that referenced this pull request Apr 4, 2019
@DonJayamanne
Merge branch 'release' into mergeIntoMaster
01332b3 
* release:
  March 2019 point release with debugger fixes (#5056)
  Update change log
  Capture telemetry when tests are disabled (#4997)
  Point release for March 2018 (#4965)
  Same logic for stable and beta (#4964)
  Release for march 2018 (#4931)
  Bump pinned version of Language Server
  Fix change log
  Ensure output panel does not steal focus due to ls errors (#4919)
  Bump ptvsd to 4.2.5 (#4904)
  Update change log entries
  Revert "Revert "Always use the same jedi environment (#4687)" (#4850)"
  cherry pick from master branch
  Cherry-pick 26a7b9c
  Fix multiline comments with text on first line. (#4884)
  fix names for run above and run below code lenses (#4879)
  Generalize test results service (#4813)
  Add ignore button to the MacInterpreterSelectedAndNoOtherInterpreters… (#4808)
  Pin to beta version of PTVSD (#4836)
  Update tpn distro and 3rd party notices
DonJayamanne added a commit that referenced this pull request Apr 8, 2019
@DonJayamanne
Merge branch 'release' into mergeRelease3
6cda7a3 
* release: (24 commits)
  Bump extension version
  Bump language server
  Track telemetry when switching to and from LS (#5164)
  Point release with fixes for Debugger (#5160)
  March 2019 point release with debugger fixes (#5056)
  Update change log
  Capture telemetry when tests are disabled (#4997)
  Point release for March 2018 (#4965)
  Same logic for stable and beta (#4964)
  Release for march 2018 (#4931)
  Bump pinned version of Language Server
  Fix change log
  Ensure output panel does not steal focus due to ls errors (#4919)
  Bump ptvsd to 4.2.5 (#4904)
  Update change log entries
  Revert "Revert "Always use the same jedi environment (#4687)" (#4850)"
  cherry pick from master branch
  Cherry-pick 26a7b9c
  Fix multiline comments with text on first line. (#4884)
  fix names for run above and run below code lenses (#4879)
  ...
@lock lock bot locked as resolved and limited conversation to collaborators Jul 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@IanMatthewHuff IanMatthewHuff IanMatthewHuff approved these changes

Assignees

@rchiodo rchiodo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rchiodo @IanMatthewHuff