👍 remove unnecessary 'foldmethod' changes #271
Conversation
WalkthroughThe changes involve modifications to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #271 +/- ##
==========================================
- Coverage 84.63% 84.60% -0.03%
==========================================
Files 63 63
Lines 3377 3372 -5
Branches 291 291
==========================================
- Hits 2858 2853 -5
Misses 517 517
Partials 2 2 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- buffer/buffer.ts (5 hunks)
- buffer/buffer_test.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- buffer/buffer.ts
buffer/buffer_test.ts
Outdated
| await t.step({ | ||
| name: "appends content of a 'foldmethod=marker' buffer", | ||
| fn: async () => { | ||
| await denops.cmd("enew"); | ||
| const bufnr = await fn.bufnr(denops); | ||
| await fn.setbufvar(denops, bufnr, "&foldmethod", "marker"); | ||
| await fn.setbufvar(denops, bufnr, "&foldmarker", "{{{,}}}"); | ||
| await append(denops, bufnr, [ | ||
| "Hello {{{", | ||
| "Darkness", | ||
| "My }}}", | ||
| "Old friend", | ||
| ]); | ||
| assertEquals([ | ||
| "", | ||
| "Hello {{{", | ||
| "Darkness", | ||
| "My }}}", | ||
| "Old friend", | ||
| ], await fn.getline(denops, 1, "$")); | ||
| assertEquals( | ||
| await fn.getbufvar(denops, bufnr, "&foldmethod"), | ||
| "marker", | ||
| ); | ||
|
|
||
| await fn.setbufvar(denops, bufnr, "&foldlevel", 0); | ||
| await append(denops, bufnr, [ | ||
| "Joking", | ||
| ]); | ||
| assertEquals([ | ||
| "", | ||
| "Joking", | ||
| "Hello {{{", | ||
| "Darkness", | ||
| "My }}}", | ||
| "Old friend", | ||
| ], await fn.getline(denops, 1, "$")); | ||
| assertEquals( | ||
| await fn.getbufvar(denops, bufnr, "&foldmethod"), | ||
| "marker", | ||
| ); | ||
|
|
||
| await fn.setbufvar(denops, bufnr, "&foldlevel", 0); | ||
| await append(denops, bufnr, [ | ||
| "Foo", | ||
| ], { | ||
| lnum: 3, | ||
| }); | ||
| assertEquals([ | ||
| "", | ||
| "Joking", | ||
| "Hello {{{", | ||
| "Foo", | ||
| "Darkness", | ||
| "My }}}", | ||
| "Old friend", | ||
| ], await fn.getline(denops, 1, "$")); | ||
| assertEquals( | ||
| await fn.getbufvar(denops, bufnr, "&foldmethod"), | ||
| "marker", | ||
| ); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Review: New test case for appending content with 'foldmethod=marker'
This test case is well-structured and covers the functionality of appending content to a buffer with the foldmethod set to "marker". It checks both the content of the buffer and the state of the foldmethod after operations, which aligns with the PR's objective to ensure functionality remains intact after removing unnecessary foldmethod changes.
However, the test could be enhanced by verifying the actual folding behavior in the buffer, not just the foldmethod setting. This would ensure that the folding functionality itself is not affected by the changes.
Would you like me to help add assertions to verify the actual folding behavior within the buffer?
buffer/buffer_test.ts
Outdated
| await t.step({ | ||
| name: "replaces content of an 'foldmethod=marker' buffer", | ||
| fn: async () => { | ||
| const bufnr = await fn.bufnr(denops); | ||
| await fn.setbufvar(denops, bufnr, "&foldmethod", "marker"); | ||
| await fn.setbufvar(denops, bufnr, "&foldmarker", "{{{,}}}"); | ||
| await replace(denops, bufnr, [ | ||
| "Hello {{{", | ||
| "Darkness", | ||
| "My }}}", | ||
| "Old friend", | ||
| ]); | ||
| assertEquals([ | ||
| "Hello {{{", | ||
| "Darkness", | ||
| "My }}}", | ||
| "Old friend", | ||
| ], await fn.getline(denops, 1, "$")); | ||
| assertEquals( | ||
| await fn.getbufvar(denops, bufnr, "&foldmethod"), | ||
| "marker", | ||
| ); | ||
|
|
||
| await fn.setbufvar(denops, bufnr, "&foldlevel", 0); | ||
| await replace(denops, bufnr, [ | ||
| "Joking {{{1", | ||
| ]); | ||
| assertEquals([ | ||
| "Joking {{{1", | ||
| ], await fn.getline(denops, 1, "$")); | ||
| assertEquals( | ||
| await fn.getbufvar(denops, bufnr, "&foldmethod"), | ||
| "marker", | ||
| ); | ||
| }, |
There was a problem hiding this comment.
Review: New test case for replacing content with 'foldmethod=marker'
This test case follows a similar structure to the appending test case, focusing on replacing content in a buffer with the foldmethod set to "marker". It effectively verifies that the content and foldmethod setting are correctly handled after the replace operation.
Like the previous test case, it would be beneficial to include checks for the actual folding behavior to ensure that the folding functionality is correctly preserved post-operation.
Would you like assistance in adding checks to verify the actual folding behavior after content replacement in the buffer?
|
Fixes #269 |
`assertEquals(actual, expected)` is correct.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- buffer/buffer_test.ts (18 hunks)
Additional comments not posted (4)
buffer/buffer_test.ts (4)
Line range hint
27-59: LGTM: Test cases for opening buffersThe test cases for opening buffers are comprehensive and cover a variety of scenarios including remote buffer names and symbols with percent-encoding. The assertions are correctly placed to ensure that the buffer properties are as expected.
Line range hint
106-198: LGTM: Test cases for decoding buffer contentThe test cases for decoding buffer content are well-structured and cover various scenarios including different encodings and file formats. The assertions correctly verify that the content and settings are as expected after decoding.
Line range hint
218-303: LGTM: Test cases for appending content to buffersThe test cases for appending content to buffers are comprehensive and check the functionality in both normal and 'unmodifiable' buffers. The assertions are correctly placed to ensure that the content is appended as expected and that buffer properties like 'modifiable' are maintained.
Line range hint
384-460: LGTM: Test cases for replacing content in buffersThe test cases for replacing content in buffers are comprehensive and check the functionality in both normal and 'unmodifiable' buffers, as well as buffers with 'foldmethod=marker'. The assertions are correctly placed to ensure that the content is replaced as expected and that buffer properties like 'modifiable' and 'foldmethod' are maintained.
| name: "appends content of a 'foldmethod=marker' buffer", | ||
| fn: async () => { | ||
| await denops.cmd("enew"); | ||
| const bufnr = await fn.bufnr(denops); | ||
| await fn.setbufvar(denops, bufnr, "&foldmethod", "marker"); | ||
| await fn.setbufvar(denops, bufnr, "&foldmarker", "{{{,}}}"); | ||
| await append(denops, bufnr, [ | ||
| "Hello {{{", | ||
| "Darkness", | ||
| "My }}}", | ||
| "Old friend", | ||
| ]); | ||
| assertEquals(await fn.getline(denops, 1, "$"), [ | ||
| "", | ||
| "Hello {{{", | ||
| "Darkness", | ||
| "My }}}", | ||
| "Old friend", | ||
| ]); | ||
| assertEquals( | ||
| await fn.getbufvar(denops, bufnr, "&foldmethod"), | ||
| "marker", | ||
| ); | ||
|
|
||
| await fn.setbufvar(denops, bufnr, "&foldlevel", 0); | ||
| await append(denops, bufnr, [ | ||
| "Joking", | ||
| ]); | ||
| assertEquals(await fn.getline(denops, 1, "$"), [ | ||
| "", | ||
| "Joking", | ||
| "Hello {{{", | ||
| "Darkness", | ||
| "My }}}", | ||
| "Old friend", | ||
| ]); | ||
| assertEquals( | ||
| await fn.getbufvar(denops, bufnr, "&foldmethod"), | ||
| "marker", | ||
| ); | ||
|
|
||
| await fn.setbufvar(denops, bufnr, "&foldlevel", 0); | ||
| await append(denops, bufnr, [ | ||
| "Foo", | ||
| ], { | ||
| lnum: 3, | ||
| }); | ||
| assertEquals(await fn.getline(denops, 1, "$"), [ | ||
| "", | ||
| "Joking", | ||
| "Hello {{{", | ||
| "Foo", | ||
| "Darkness", | ||
| "My }}}", | ||
| "Old friend", | ||
| ]); | ||
| assertEquals( | ||
| await fn.getbufvar(denops, bufnr, "&foldmethod"), | ||
| "marker", | ||
| ); |
There was a problem hiding this comment.
Review: Test cases for 'foldmethod=marker' buffer operations
These test cases are crucial for ensuring that the buffer operations do not interfere with the 'foldmethod' settings, especially after the changes made in this PR. The tests verify that the content is appended or replaced correctly and that the 'foldmethod' remains set to "marker".
Suggestions:
- Verify Actual Folding Behavior: As previously suggested, it would be beneficial to include assertions that verify the actual folding behavior within the buffer. This would ensure that the folding functionality itself is not affected by the changes.
- Enhance Test Coverage: Consider adding scenarios where the folding might interact differently due to the content changes, such as nested folds or folds that include line breaks.
Would you like assistance in enhancing these tests to cover the suggestions mentioned?
I also added tests, but I looks like no problem, so I think it's okay to delete these tests.
Summary by CodeRabbit
New Features
foldmethodduring append and replace actions.foldmethodset to "marker".Bug Fixes
foldmethodremains unchanged during operations.