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 SyntaxAt and SyntaxOf helpers. #16

Merged
merged 1 commit into from Sep 14, 2014

Conversation

Projects
None yet
3 participants
@cirosantilli
Contributor

cirosantilli commented Sep 7, 2014

With this, even mere human beings can test syntax files. =)

The only thing I haven't done is document under doc/, only under README.md. At a quick look it seems like both are the same: are you manually doing things twice, or is there a better way?

@junegunn

This comment has been minimized.

Owner

junegunn commented Sep 7, 2014

Thanks, but I'm not sure if want to merge this feature since I want to keep Vader as simple as possible. Actually I'm not a fan of helper functions in general as I believe they tend to make the project unnecessarily complex in the long run. How about if we put the definition of the functions in the wiki page instead of providing them as the Vader default? I prefer to define project-wise helper functions and commands and avoid extending Vader itself.

Implementation-wise there are several points I disagree with

  • The order of the parameters in SyntaxAt(col, [line=1]) does not follow the convention shared by many Vim functions where column number comes after line number. It can be confusing.
  • vader#helper#syntax_at_pattern only looks at the first match, what if I want to check the other matches as well?
  • It also has the side-effect of moving the cursor, which can affect the result of the assertions that follow.
@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Sep 7, 2014

If you think it's too much for Vader, please close the PR.

I believe:

  • this is something every project which wants to test syntax highlighting will need to have. I came to Vader specifically to test syntax highlighting: there might be others who do too.
  • it plays nicely with the existing embedded syntax highlight Vader has

If we put it on the wiki, it means that several projects would need to copy paste extra code. If we put it in another plugin, it means that people would have to do extra searching to find that the other plugin exists.

About the implementation:

  1. The advantage of this order is that you type less for single line test cases, which should be pretty common in minimalistic tests, but we can change it if you think it's not worth it. Agree it is not the convention.
  2. The current function does well for small samples of text, which should be a common case. But it can be easily extended with a third optional argument that gets the nth match if you require. There is also the workaround of using syntax_at instead.
  3. Agree. I don't know how to solve this kind of problem well. How do you usually do it? The only alternatives I know of are:
  • save it on a variable and then jump back. But this makes the jump list dirty
  • use search. But this starts from current cursor position.

Since both are imperfect, and state should not matter much for minimal test cases, I did nothing. Another option is to simply document this fact.

@junegunn

This comment has been minimized.

Owner

junegunn commented Sep 7, 2014

I agree to a certain degree that syntax-helpers can be a good addition to Vader. I'll think about it and let you know. As the maintainer of the project, I have to be give extra care when extending the API. Once released, it's never easy to withdraw the decision.

The advantage of this order is that you type less for single line test cases, which should be pretty common in minimalistic tests, but we can change it if you think it's not worth it. Agree it is not the convention.

I would expect it to return the syntax highlight group of the current cursor position when no argument is given.

Execute (line 2, col 10):
  normal! 2G10|
  AssertEqual 'String', SyntaxAt()
  " Go back if necessary
  normal! ``

  " Or equivalently
  AssertEqual 'String', SyntaxAt(2, 10)

  " Current line: line('.') or just '.'
  AssertEqual 'String', SyntaxAt('.', 10)

Execute:
  call search('some_pattern', 's')
  AssertEqual 'String', SyntaxAt()

  " Or equivalently
  AssertEqual 'String', SyntaxAt('.', '.')

  " Go back if necessary
  normal! ``

The current function does well for small samples of text, which should be a common case. But it can be easily extended with a third optional argument that gets the nth match if you require. There is also the workaround of using syntax_at instead.

Hmm, I don't think it's easy to come up with a complete API that covers most of the use cases. The user may want to search from the beginning, or from the cursor position. Maybe we should just remove SyntaxAtPattern and let the user does the search himself as shown in the above example. But let's not rush into decision.

Agree. I don't know how to solve this kind of problem well. How do you usually do it?

cursor() will not change jumplist, or you can use :keepjumps. Also setpos() does not change jumplist.

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Sep 7, 2014

I agree to removing SyntaxAtPattern, and allowing . on SyntaxAt as on the above examples.

Let me know if you decide this is worth it so I can patch it. I understand your concern about not adding unnecessary features: it's an NP complete question =)

@junegunn

This comment has been minimized.

Owner

junegunn commented Sep 7, 2014

What do you think about adding an assertion instead? AssertSyntax or AssertHighlight

# AssertSyntax EXPECTED[, LINE, COL]
Execute:
  normal! 2G10|
  AssertSyntax 'String'
  AssertSyntax 'Comment', 2, 10

  call search('pattern')
  AssertSyntax 'Function'
@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Sep 7, 2014

At first I was going to do that, but then I went for the helper, since:

  • it is very easy to do AssertSyntax once you have the helper. Somewhat the same reason why AssertPattern was removed.
  • the helper is strictly more powerful as it does not force the combination of both operations and can more easily be recombined. For instance, we would have to add yet another Assert for the negation: AssertNotSyntax.

This is specially bad as more helpers are added in the future.

True, AssertSyntax is going to be the major use case of SyntaxAt, so it's a close call. But I'd rather go for more sustainable atomic functions.

@junegunn

This comment has been minimized.

Owner

junegunn commented Sep 8, 2014

Okay, here is what I suggest:

SyntaxAt

  • SyntaxAt() - Syntax of the current cursor position
  • SyntaxAt(col) - Syntax of the column on the current line. (line number alone doesn't really make sense, so I think it's okay to take column number in this case)
  • SyntaxAt(line, col) - Conventional function signature

SyntaxOf

  • SyntaxOf(pattern[, index = 1])
    • The optional index parameter to look at the n-th match (from the beginning). If there's no n-th match, the function should return an empty string.
    • The cursor should not change its position

Test

  • Please add test case for each function signature
  • Fix Travis-CI build
    • Adding syntax enable to .travis.yml should do, I guess

Documentation

  • Please keep it short and simple. Remove the statusline example you added to the page.
  • Don't worry about the vimdoc, I use a script to convert markdown into vimdoc.

Etc

  • SyntaxAt and SyntaxOf can be Funcrefs. e.g. let SyntaxAt = function('vader#helper#...')
@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Sep 9, 2014

OK, I'll patch when I get some time.

What is the advantage of using Funcrefs over regular functions? First time I see them and Google did not help.

@junegunn

This comment has been minimized.

Owner

junegunn commented Sep 9, 2014

Not that I'm aware of. But I think it's conceptually cleaner/simpler to use funcref in this case instead of defining another function just to pass the arguments. It's also more concise.

@cirosantilli cirosantilli force-pushed the cirosantilli:syntax-at branch from d3b01bf to 574302d Sep 13, 2014

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Sep 13, 2014

Updated.

I had to use let g:SyntaxAt with g: for it to find the function with the funcref. Not sure what is the minimum scope that would work.

@junegunn

This comment has been minimized.

Owner

junegunn commented Sep 13, 2014

Thanks! I'll leave some comments on the commit so please take a look. I'll merge it tomorrow after review.

@cirosantilli cirosantilli force-pushed the cirosantilli:syntax-at branch from 574302d to eb13af8 Sep 13, 2014

@cirosantilli cirosantilli force-pushed the cirosantilli:syntax-at branch from eb13af8 to 3767179 Sep 13, 2014

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Sep 13, 2014

Updated.

junegunn added a commit that referenced this pull request Sep 14, 2014

Merge pull request #16 from cirosantilli/syntax-at
Add SyntaxAt and SyntaxAtPattern helpers.

@junegunn junegunn merged commit f4eab86 into junegunn:master Sep 14, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@junegunn

This comment has been minimized.

Owner

junegunn commented Sep 14, 2014

Merged, thanks!

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Sep 14, 2014

Thank you and sorry for so many mistakes! Finally I can add some decent tests to Vim markdown...

@cirosantilli cirosantilli deleted the cirosantilli:syntax-at branch Sep 14, 2014

@cirosantilli cirosantilli changed the title from Add SyntaxAt and SyntaxAtPattern helpers. to Add SyntaxAt and SyntaxOf helpers. Sep 14, 2014

@justinmk

This comment has been minimized.

justinmk commented Sep 14, 2014

I use a script to convert markdown into vimdoc.

@junegunn Could you share this script if I promise not to make bug/feature requests? :)

@junegunn

This comment has been minimized.

Owner

junegunn commented Sep 15, 2014

@justinmk Maybe someday when I get to clean things up. It's currently anywhere near the quality I would like it to be. Bunch of super-horrible hacks 💩

@justinmk

This comment has been minimized.

justinmk commented Sep 15, 2014

Fair enough!

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