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

Add automatic formatting of multi-line `stripMargin` strings on paste #968

Merged
merged 2 commits into from Oct 14, 2019

Conversation

@ckipp01
Copy link
Member

ckipp01 commented Oct 6, 2019

Fixes #952

metals

I left a comment on the issue as I wasn't sure the way to go about testing this -- mainly where to put the test or if I should tack it on to another. Any pointers there would be helpful. I also may have missed some helper methods or ways I could have reused stuff, so please let me know! I figured I'd make the pr right away in order to get some feedback.

@tgodzik

This comment has been minimized.

Copy link
Collaborator

tgodzik commented Oct 6, 2019

Thanks for the contribution, will take a look as soon as I can!

@ckipp01 ckipp01 changed the title add ability to paste multi-line strings in from other multi-line strings add ability to paste multi-line strings in from other multi-line strings with correct formatting Oct 6, 2019
Copy link
Collaborator

tgodzik left a comment

Seems that Github actions failed, not sure why.

The code looks nice, but for sure you need to check RangeFormattingSuite and it might be failing now.

You can run it using sbt:

sbt unit/testOnly tests.RangeFormattingSuite
@ckipp01

This comment has been minimized.

Copy link
Member Author

ckipp01 commented Oct 8, 2019

The code looks nice, but for sure you need to check RangeFormattingSuite and it might be failing now.

Sounds good. I double-checked that the tests were passing and also adding one that specifically checks the behavior of pasting a multilineString into another multilineString.

@ckipp01 ckipp01 requested a review from tgodzik Oct 9, 2019
Copy link
Collaborator

tgodzik left a comment

Thanks for fixing, I would still add some more tests to check this out.

@ckipp01 ckipp01 force-pushed the ckipp01:master branch 2 times, most recently from 5b0d559 to 6edf345 Oct 11, 2019
@ckipp01 ckipp01 requested a review from tgodzik Oct 11, 2019
Copy link
Collaborator

tgodzik left a comment

Thanks for the additional tests! Just one comment - we can simplify the branches a lot in the code.

@ckipp01 ckipp01 force-pushed the ckipp01:master branch from 6edf345 to 96a910a Oct 11, 2019
@ckipp01 ckipp01 requested a review from tgodzik Oct 11, 2019
@olafurpg olafurpg changed the title add ability to paste multi-line strings in from other multi-line strings with correct formatting Add automatic formatting of multi-line `stripMargin` strings on paste Oct 13, 2019
Copy link
Member

olafurpg left a comment

Thank you for this contribution! Just tried this PR locally and it works great, looking forward to enjoy this fix.

Left a couple minor nitpick comments. Otherwise, looks good to me.

PS. I recommend using a non-master branch for opening pull requests in the future. This makes it easier for maintainers to push additional commits to your open PR, if necessary.

@ckipp01 ckipp01 force-pushed the ckipp01:master branch from 96a910a to f64d211 Oct 14, 2019
@ckipp01 ckipp01 requested a review from olafurpg Oct 14, 2019
Copy link
Member

olafurpg left a comment

LGTM 👍 Thank you!

@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Oct 14, 2019

I restarted the GH Actions job since it was timing out and also opened #988 to re-introduce Azure pipelines to see if it is more stable.

@tgodzik

This comment has been minimized.

Copy link
Collaborator

tgodzik commented Oct 14, 2019

I am going to merge this - github actions seem badly broken currently.

@tgodzik tgodzik merged commit 76c85c1 into scalameta:master Oct 14, 2019
1 of 2 checks passed
1 of 2 checks passed
build build
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.