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 windows line endings #109

Closed
wants to merge 2 commits into from

Conversation

ZNielsen
Copy link
Collaborator

@ZNielsen ZNielsen commented Oct 6, 2021

Check list

  • I have read through the README (especially F.A.Q section)
  • I have searched through the existing issues or pull requests
  • I have verified all existing unit tests pass (See the README on how to run)
  • I have added new tests if appropriate
  • I have performed a self-review of my code and commented hard-to-understand areas
  • I have made corresponding changes to the documentation (when necessary)

Description

A couple places assumed unix line endings, but the Search function/unit
tests heavily relied on it. This commit detects the platform and changes
the used line endings accordingly.

Fixes #101

Type of change

  • Bug fix
  • New feature
  • Improvement of existing features
  • Refactor
  • Breaking change
  • Documentation change
  • CI / CD

Test environment

  • OS
    • Linux
    • Mac OS X
    • Windows
    • Others:
  • Vim
    • Neovim: 0.5
    • Vim:

@ZNielsen ZNielsen added the hacktoberfest-accepted For hacktoberfest PRs label Oct 6, 2021
@ZNielsen
Copy link
Collaborator Author

ZNielsen commented Oct 6, 2021

I do not have access to a Windows machine to test. Is someone else able to check that the unit tests now pass in Windows?

@ZNielsen ZNielsen requested a review from wfxr October 6, 2021 22:18
@ZNielsen
Copy link
Collaborator Author

ZNielsen commented Oct 6, 2021

@garyttierney, this was exposed on a PR you posted up - if you have time, could you give the unit tests a check?

@garyttierney
Copy link
Contributor

Hey @ZNielsen, sorry, missed this notification. I'll check it later today.

@ZNielsen
Copy link
Collaborator Author

No worries, open source happens on its own timeline.

Copy link
Owner

@wfxr wfxr left a comment

Choose a reason for hiding this comment

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

LGTM

@garyttierney
Copy link
Contributor

Hm, the minimap does seem to work correctly (the highlighted region looks good for beginning, middle, and end of a 100 KiB file) in my editor, but the tests are still failing:

✗ Search - Beginning of file
	Expected "[]" to equal "[[1, 1, 3], [3, 4, 3], [3, 10, 3], [3, 13, 3]]"
		<SNR>48_minimap_test_search_beginning_of_file:5
✗ Search - Beginning of line
	Expected "[]" to equal "[[1, 4, 3], [3, 1, 3], [3, 1, 3]]"
		<SNR>48_minimap_test_search_beginning_of_line:7
✗ Search - End of line
	Expected "[]" to equal "[[1, 4, 3], [3, 7, 3]]"
		<SNR>48_minimap_test_search_end_of_line:5
✗ Search - Multi match per line
	Expected "[]" to equal "[[3, 4, 3], [3, 16, 3], [3, 22, 3]]"
		<SNR>48_minimap_test_search_multi_per_line:5
√ Search - Spanning a line
✗ Search - Many results
	Expected "[]" to equal "[[1, 1, 3], [1, 4, 3], [1, 19, 3], [1, 22, 3], [1, 25, 3], [1, 25, 3], [1, 1, 3], [1, 1, 3], [1, 1, 3], [2, 1, 3], [2, 1, 3], [2, 1, 3], [2, 1, 3], [3, 1, 3], [3, 1, 3], [3, 1, 3], [3, 1, 3], [3, 1, 3], [3, 16, 3]]"
		<SNR>48_minimap_test_search_many_results:8
✗ Search - Long Match
	Expected "[]" to equal "[[1, 7, 21]]"
		<SNR>48_minimap_test_search_long_match:5
✗ Search - History
	Expected "[]" to equal "[[1, 1, 3], [1, 1, 3], [1, 1, 3], [2, 1, 3], [2, 1, 3], [2, 1, 3], [2, 1, 3], [3, 1, 3], [3, 1, 3]]"
		<SNR>48_minimap_test_search_history:11

A couple places assumed unix line endings, but the Search function/unit
tests heavily relied on it. This commit detects the platform and changes
the used line endings accordingly.
@rabirabirara
Copy link
Collaborator

I tried the changes in Windows (plus the new unit test file) and only one unit test does not pass:

Search - History
	Vim(let):E866: (NFA regexp) Misplaced @
		<SNR>80_minimap_test_search_history:10

It can be fixed by commenting out the changes in the function, minimap#...color_search_get_spans(); specifically from line 795:

let split_char = '\n'
" if has('win32')
"     let split_char = '\r\n'
" endif
let lines = split(git_diff, split_char)

This is the one change with notable problems. For example, it also broke git coloring. This is a screenshot of a dos formatted file to which I made some random changes to and saved. I disabled the line ending changes, and the git coloring works:

image

If I re-enable those specific changes, the highlighting breaks.

image

However, I also tried disabling the other line ending changes, and I didn't see any improvement upon making the changes from \n to \r\n. For example, if highlight search didn't work on my Windows machine before, it didn't work post-changes.

Despite writefile() not distinguishing the endings, I found it to work correctly on my Windows machine when creating the test file for t/search_tests.vim. (Well, probably because it's designed to work in C, while Vimscript is just kind of obtuse in comparison.)

Since the problem discussed in the PR was solved by using the Vimscript function writefile(), there shouldn't be a problem with those unit tests anymore. Due to the above reasons, I actually recommend withholding these changes for now.

Apologies for my long absence from this PR. If someone else with a Windows machine has a different experience, please follow up!

@stale
Copy link

stale bot commented Apr 15, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 15, 2022
@stale stale bot closed this May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted For hacktoberfest PRs stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search highlighting does not work on Windows
4 participants