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

feat: Insert Missing Comma on Enter #6

Merged
merged 47 commits into from
Nov 23, 2022
Merged

feat: Insert Missing Comma on Enter #6

merged 47 commits into from
Nov 23, 2022

Conversation

AgentRBY
Copy link
Contributor

Fix #5

P.s.: I have not written extensions in VS Code before, so I am open to suggestions

@AgentRBY AgentRBY changed the title Add "Insert Comma" Add "Insert Comma After Enter" option Nov 19, 2022
package.json Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
@zardoy
Copy link
Owner

zardoy commented Nov 19, 2022

Wow looks interesting! I think current implementation is fine enough, but probably only for json without comments. Initially I was thinking of using some parsing method to insert comma more precisely, some cases to test:

I marked with | positions where comma would be inserted incorrectly.

{
  "key": "value" /* comment */ // case 1|
}

Btw I checked that WebStorm doesn't do this auto comma inserting after comments and don't think its right.

I think we can easily workaround this comments problem just by proceeding the editor text with strip-json-comments module.

Btw I see I didn't check this project for a long time, so I'll try to setup a decent CI soon.

package.json Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
@AgentRBY
Copy link
Contributor Author

AgentRBY commented Nov 20, 2022

Thx for feedback!
I fixed everyting exept startsWith because regex is very slow.

I think we can easily workaround this comments problem just by proceeding the editor text with strip-json-comments module.

Also added JSONC support with "strip-json-comments"

In multicursor mode i have some problems:

  1. prevLinePosition did not point to the last character, so I create a new Position
  2. In some cases the comma is not inserted (see gif) and IDK how fix that
    Code_KY8XwsBlf1

@zardoy zardoy changed the title Add "Insert Comma After Enter" option Add "Insert Missing Comma On Enter" Nov 20, 2022
Copy link
Owner

@zardoy zardoy left a comment

Choose a reason for hiding this comment

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

I pushed some minor changes, so be sure to check them! Other things I leave to you for now! I also did some changes on main branch, so could also merge or rebase them?

src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
@zardoy zardoy added the enhancement New feature or request label Nov 20, 2022
@zardoy
Copy link
Owner

zardoy commented Nov 20, 2022

Also what about case that I mentioned above:

{
    "key": "value" //comment
}

I think its still would be good to handle cases like this (insert comma after "value"). What do you think?

I think these requested changes are final, just need to figure out why CI checks don't run on this pull request? 🤔 Maybe you need to reopen the pr?

src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
@AgentRBY
Copy link
Contributor Author

Also what about case that I mentioned above:

{
    "key": "value" //comment
}

I think its still would be good to handle cases like this (insert comma after "value"). What do you think?

Done.

I think these requested changes are final, just need to figure out why CI checks don't run on this pull request? 🤔 Maybe you need to reopen the pr?

Maybe, IDK. After all the issues are resolved, I will try to close this PR and create a new one

src/extension.ts Outdated Show resolved Hide resolved
@AgentRBY AgentRBY requested a review from zardoy November 22, 2022 18:57
@zardoy
Copy link
Owner

zardoy commented Nov 22, 2022

Hey, @AgentRBY I've pushed integration tests and also fixes for them. Can you please wether they are good for you? I tried to to cover only basic cases, but probably there are any other cases that you would like to cover?

UPD: Also I removed the check that current line starts with comment. I didn't remember what cases it was covering? Gonna be honest, I don't think we ever need startsWithComment function here.

Meanwhile I'll try to fix CI issues here.

@AgentRBY
Copy link
Contributor Author

Hey, @AgentRBY I've pushed integration tests and also fixes for them. Can you please wether they are good for you? I tried to to cover only basic cases, but probably there are any other cases that you would like to cover?

I've added some other simple tests to test more cases
Also, I think we should add tests for the pasting after comment (as in gif):
Code_pJ2Ftmtsu3
But I haven't figured out how to do it, so I'll leave it to you, @zardoy

UPD: Also I removed the check that current line starts with comment. I didn't remember what cases it was covering? Gonna be honest, I don't think we ever need startsWithComment function here.

It was needed rather for optimization, in order to immediately discard values ​​that begin with comments. In general, its removal does not harm anything.

@zardoy
Copy link
Owner

zardoy commented Nov 23, 2022

Also, I think we should add tests for the pasting after comment

I was going to add them as well, but forgot about, thanks for pointing!

@zardoy
Copy link
Owner

zardoy commented Nov 23, 2022

@AgentRBY I misread your comment:

I've added some other simple tests to test more cases
Also, I think we should add tests for the pasting after comment (as in gif):

I didn't get the idea about what you were talking, as I can see in the gif, you're just doing ctrl+Enter or Enter at the end. We already have test for this: https://github.com/zardoy/vscode-fix-all-json/pull/6/files#diff-cfd18f0b3c283dd23cf5513db00647d1949112658b52c7f406359cc92acdec00R19

Copy link
Owner

@zardoy zardoy left a comment

Choose a reason for hiding this comment

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

All LGTM! Can I merge, or you want anything else to add, @AgentRBY ?

@zardoy zardoy changed the title Add "Insert Missing Comma On Enter" feat: Insert Missing Comma On Enter Nov 23, 2022
@zardoy zardoy changed the title feat: Insert Missing Comma On Enter feat: Insert Missing Comma on Enter Nov 23, 2022
@AgentRBY
Copy link
Contributor Author

I didn't get the idea about what you were talking, as I can see in the gif, you're just doing ctrl+Enter or Enter at the end. We already have test for this: https://github.com/zardoy/vscode-fix-all-json/pull/6/files#diff-cfd18f0b3c283dd23cf5513db00647d1949112658b52c7f406359cc92acdec00R19

Okay, I misunderstood the test you pointed out a bit. I just checked and it's really what I wanted

I think it's ready to merge @zardoy

@zardoy zardoy merged commit 883dda4 into zardoy:main Nov 23, 2022
@zardoy
Copy link
Owner

zardoy commented Nov 23, 2022

@AgentRBY Thank you so much for the idea & implementation 🎉. I'm happy that now I have solution when editing JSON in the web. Do you want to handle a similar problem: #9 ?

@AgentRBY
Copy link
Contributor Author

@AgentRBY Thank you so much for the idea & implementation 🎉. I'm happy that now I have solution when editing JSON in the web. Do you want to handle a similar problem: #9 ?

Okay i will do it

Also check #10, if you like the idea, I will implement it too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically insert a comma on Enter
2 participants