Skip to content

Only set 'syn-sync' when it is the main syntax #73

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

Merged
merged 2 commits into from
Dec 6, 2020
Merged

Only set 'syn-sync' when it is the main syntax #73

merged 2 commits into from
Dec 6, 2020

Conversation

pjio
Copy link
Contributor

@pjio pjio commented Dec 2, 2020

No description provided.

@pjio
Copy link
Contributor Author

pjio commented Dec 2, 2020

This PR tries another approach and sets a flag in the files which include the nested syntax.

I tested with nvim v0.4.3 and disabled my other plugins:

:syn sync shows for graphql files as intended:
syncing starts 500 lines before top line

For php files :syn sync show the original:
syncing starts 2147483647 lines before top line

I also tested that the nested syntax is still working. Very nice feature to have!

Copy link
Owner

@jparise jparise left a comment

Choose a reason for hiding this comment

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

Do you think there's a good way to write a test for this behavior to prevent future regressions?

@pjio
Copy link
Contributor Author

pjio commented Dec 2, 2020

I changed the condition to if !get(b:, 'graphql_nested_syntax')

To write a unit test is more tricky than I first thought. The problem is to set minlines to a predictable value before the buffer is loaded.

I had some success with adding this to test/php/default.vader

Execute (Settings assertions):
  redir @a | syntax sync | redir END
  AssertEqual 'syncing starts 2147483647 lines before top line', @a

But on the CI there are different defaults:
image

For now I think it's not easily possible.

@pjio
Copy link
Contributor Author

pjio commented Dec 2, 2020

I added a test which checks for the '500' in the syntax setting.

@pjio
Copy link
Contributor Author

pjio commented Dec 3, 2020

The test checks now for the exact value which would be set if there is a regression. The downside is, the test has to be changed whenever the value "500" is changed or the string format changes. The upside is, it shouldn't fail as a false positive. I had to store the result of match() in a local variable because Assert didn't handle the comparison as a boolean expression (I assume a limitation of Vader..)

Copy link
Owner

@jparise jparise left a comment

Choose a reason for hiding this comment

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

This looks good! I'll give it a final look over the weekend and then merge.

@jparise jparise merged commit 2e3b8fb into jparise:master Dec 6, 2020
@jparise
Copy link
Owner

jparise commented Dec 6, 2020

Thanks for this, @pjio! I appreciate you digging in to the issue and producing a generalized solution to the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants