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 unwanted extra spaces when pasting code with JSDoc #136579

Merged
merged 7 commits into from
Nov 18, 2022

Conversation

ssigwart
Copy link
Contributor

@ssigwart ssigwart commented Nov 6, 2021

This PR fixes #119225. It prevents an extra space from being inserted after JavaDoc style comments.

To test, you can paste something like this into a typescript file.

/**
 * Function
 */
public func1(): number
{
	return 1;
}

I also added a test that can be run with:

./scripts/test.sh --glob **/indentation.test.js --grep=119225

@ssigwart
Copy link
Contributor Author

ssigwart commented Nov 7, 2021

I realized the unit test didn't actually work properly, so I re-wrote it and updated the comment above with how to run it.

@ssigwart
Copy link
Contributor Author

When someone does review this, I just wanted to point out that reviewing with ignored whitespace will make it easier.

@ssigwart
Copy link
Contributor Author

ssigwart commented Jun 5, 2022

I rebased this to fix merge conflicts. It is ready for review.

@jespertheend
Copy link

🥲

@ssigwart
Copy link
Contributor Author

ssigwart commented Jul 2, 2022

I fixed the merge conflicts again. Hopefully this can get reviewed soon.

@otacke
Copy link

otacke commented Nov 15, 2022

@hediet @sandy081 Sorry if I am disturbing you. I just picked you, because you seem to be committing a lot, even though you may not been responsible.

This pull request seems to be a fix for a bug #119225 which is very annoying. The pull request has been open and actively maintained for over a year. Could you please give any insight into your working process in order to shed some light onto when the bug might eventually be fixed? Is there anything I could do to contribute?

@hediet
Copy link
Member

hediet commented Nov 16, 2022

What happens with this PR, when you paste this:

/**
 * Function
 */
public func1(): number
{
	return 1;
}

into a class that already has some indentation?
Does the pasted code get indented correctly?

@otacke
Copy link

otacke commented Nov 16, 2022

@hediet

What happens with this PR, when you paste this:

/**
 * Function
 */
public func1(): number
{
	return 1;
}

into a class that already has some indentation? Does the pasted code get indented correctly?

Is this a general question or addressing me? In the latter case, I need to amend my previous comment about the information that I have not built a version of VScode with the PR merged in and therefore deliberately wrote "pull-request seems to be a fix". I am confident, however, that @ssigwart would answer the question given that he contributed the code.

@ssigwart
Copy link
Contributor Author

Thanks for responding, @hediet!

The bug is that when you paste the code you quoted in a JavaScript file, it shows up like this:

/**
 * Function
 */
 public func1(): number
 {
	 return 1;
 }

Notice the extra spaces in front of "public" and the lines below it.

With this PR, it gets pastes without adding the spaces. I think you are might be asking specifically if it will still function properly if you try to paste it where you expect an extra tab in front of every line. It's been a long time since I wrote this, but I'm fairly certain that works too. I can try to verify that tonight when I get back onto the computer I wrote this patch on.

@ssigwart
Copy link
Contributor Author

@hediet, I tested my branch. If there's existing indentation, it behaves the same way as before this change, so it seems good to me for this branch. It looks like the bug with the extra spaces only happens when there's no indentation and that's what this PR fixes.

@jespertheend
Copy link

Fwiw I'm also seeing an extra space when there is already an indentation, and if the line being pasted at is already indented formatting gets even more broken.

To be clear, this recording was made in the latest stable version of vscode that does not contain the fix of this PR yet.

Screen.Recording.2022-11-17.at.11.59.49.mp4.mp4

@ssigwart
Copy link
Contributor Author

@jespertheend, thanks for the recording. I'm not seeing the same behavior on the stable version of VS Code. Maybe it's different settings.

Can you include two things so I can test it better?

  1. The exact raw JS that you start with.
  2. The exact raw JS that's in your clipboard. In particular, I'm curious if the text in the clipboard has tabs already or not.

@jespertheend
Copy link

I'm seeing the same behaviour in an incognito window on vscode.dev.
My exact javascript file can be downloaded here: Foo.zip

To reproduce:

  • drag Foo.js to an incognito instance of vscode.dev
  • click a character on line 6
  • press Ctrl + L 7 times
  • hit Ctrl + X followed by Ctrl + V

@jespertheend
Copy link

Ah and my clipboard contains this (encoded as javascript string)

'    /**\n     * @param {number} x \n     * @param {string} y \n     */\n    bar(x, y) {\n\n    }\n'

@ssigwart
Copy link
Contributor Author

Thanks, @jespertheend. I tested with my branch and it does solve that as well.

@hediet hediet added this to the November 2022 milestone Nov 18, 2022
@hediet hediet merged commit ec0ab31 into microsoft:main Nov 18, 2022
@otacke
Copy link

otacke commented Nov 18, 2022

@hediet Thanks a lot for reacting to my request to have a look at this pull request! And some extra thanks for doing it so quickly. I really appreciate that.

@jespertheend
Copy link

I've been waiting for this moment for so long 🥲

@ssigwart ssigwart deleted the jsdoc2 branch November 18, 2022 13:22
@ssigwart
Copy link
Contributor Author

Thank you so much, @hediet and @joaomoreno.

What is the proper way to get attention on PRs? I have several that are over a year old where I haven't heard any feedback. In particular, there are 3 about code formatting (#136572, #136577, #136593) that would fix some annoying issues that I and I'm guessing others deal with daily.

@hediet
Copy link
Member

hediet commented Nov 21, 2022

I can try to look at some of them some time.
Please ping me in the formatting-related PR that you would like to have the most.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
editor-autoindent Editor auto indentation issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pasting into code line below a jsdoc block comment auto-indents by one space character
9 participants