Skip to content
This repository has been archived by the owner on Apr 1, 2020. It is now read-only.

Add indents to every line in the snippet. #2394

Merged
merged 3 commits into from
Jul 8, 2018

Conversation

CrossR
Copy link
Member

@CrossR CrossR commented Jul 3, 2018

Fixes #2242.

Adds the correct indent level to every line in a snippet.

Is there any edge cases I'm missing where this could fall apart?

@codecov
Copy link

codecov bot commented Jul 3, 2018

Codecov Report

Merging #2394 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2394      +/-   ##
==========================================
+ Coverage    38.2%   38.23%   +0.03%     
==========================================
  Files         300      300              
  Lines       12518    12530      +12     
  Branches     1649     1650       +1     
==========================================
+ Hits         4782     4791       +9     
- Misses       7481     7484       +3     
  Partials      255      255
Impacted Files Coverage Δ
browser/src/Services/Snippets/SnippetSession.ts 81.33% <100%> (+1.05%) ⬆️
browser/src/UI/components/Tabs.tsx 30.84% <0%> (-0.23%) ⬇️
...src/Services/Configuration/DefaultConfiguration.ts 87.5% <0%> (ø) ⬆️

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 d0e8898...59d52c4. Read the comment docs.

akinsho
akinsho previously approved these changes Jul 3, 2018
Copy link
Member

@akinsho akinsho left a comment

Choose a reason for hiding this comment

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

Excellent @CrossR 👍 thats been a real annoyance

@akinsho
Copy link
Member

akinsho commented Jul 4, 2018

@CrossR I think if the snippets are subdivided by line then adding an indent should be fine worst case a line might wrap because of indentation but I dont think anything would blow up, also from the indent lines plugin Im pretty sure detect indent handles empty lines fine as well

@CrossR
Copy link
Member Author

CrossR commented Jul 4, 2018

Just gave this an actual test (vs just a visual test of the lines) and realised the way I was doing it wasn't updating the placeholders, so tabbing through left you in the old positions.

I'm pushing up a change for that now, that fixes it on my end.

@bryphe
Copy link
Member

bryphe commented Jul 5, 2018

Nice catch @CrossR ! It would be worth having a test case to document this behavior - we have some examples here:

it("matches existing whitespace - tabs", async () => {

(I think it'd be pretty similar to that test, except we'd want to have some whitespace in the buffer - all the tests at the moment just insert a snippet to an empty buffer).

@CrossR
Copy link
Member Author

CrossR commented Jul 8, 2018

Unit tests added now, is it worth adding some for the tabbing through placeholders? Technically I didn't actually change any of that, since the stuff I've added is all done before that.


const [firstLine, secondLine, thirdLine] = await mockBuffer.getLines(0, 3)

assert.strictEqual(firstLine, "\tfor {")
Copy link
Member

Choose a reason for hiding this comment

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

Nice! Thanks for adding these tests! 💯

@bryphe
Copy link
Member

bryphe commented Jul 8, 2018

Unit tests added now, is it worth adding some for the tabbing through placeholders? Technically I didn't actually change any of that, since the stuff I've added is all done before that.

This is great! Just glad we have the indentation behavior covered. I think if we find bugs with the placeholder behavior we could add some additional ones to cover it, at that time.

@CrossR CrossR merged commit 02dd6f2 into onivim:master Jul 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants