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 support for setext headers to the TOC command. #80

Merged
merged 1 commit into from Jun 3, 2014

Conversation

Projects
None yet
2 participants
@fmoralesc
Contributor

fmoralesc commented May 4, 2014

I took the base of your TOC function for vim-pandoc/vim-pantondoc, adding setext headers support. I thought it would be nice if I shared that back (the full implementation is here: https://github.com/vim-pandoc/vim-pantondoc/blob/master/autoload/pantondoc/toc.vim, you'll see there are some additional differences). Feel free to clean it up.

let l:line = substitute(l:line, '\v[ ]*#*$', '', '')
let l:line = repeat(' ', (2 * l:length)) . l:line
call setline(i, l:line)
" this is the quickfix data for the current item

This comment has been minimized.

@cirosantilli

cirosantilli Jun 2, 2014

Collaborator

Please use spaces only instead of tabs.

@cirosantilli

This comment has been minimized.

Collaborator

cirosantilli commented Jun 2, 2014

Thanks for your contrib.

Please remove the:

The following commands currently only work for atx style headers (#). Pull request are welcome to extend them to Setext style headers (===).

note from the README.

@fmoralesc

This comment has been minimized.

Contributor

fmoralesc commented Jun 2, 2014

Updated the patch with the requested changes. Also, cleaned up the commit message.

@cirosantilli

This comment has been minimized.

Collaborator

cirosantilli commented Jun 2, 2014

Thanks for updating.

I have tried it locally and it did not work.

How are you putting the Setex headers into the quickfix? It is done via silent vimgrep '^#' % for Atx.

@fmoralesc

This comment has been minimized.

Contributor

fmoralesc commented Jun 2, 2014

Oops, forgot to update the vimgrep command. Should be fixed now.

@cirosantilli

This comment has been minimized.

Collaborator

cirosantilli commented Jun 2, 2014

I'm testing with: https://github.com/cirosantilli/vim-markdown/blob/setex-tests/test/multiple_headers.md + https://github.com/cirosantilli/vundle-plugin-tester

  1. Why add abort? If it is something that should be done for all functions, please make another pull request. This is the first time I see it, so I'm not sure about this point.

  2. The quickfix indent for h2 headers is now 1 space instead of two. Please keep the old behavior.

  3. This breaks Atx headers without a space after the #: #header because of the space added on the regex at #\+.

  4. Please squash and make this into a single commit.

  5. Setex h1 headers are generating a blank line on the quickfix as in:

    h1 after h2
    
    setex h1
    

    Please remove added empty lines.

  6. Why add the \%^% part of the regex? Is it to capture the title of the Pandoc frontmatter extension? If that is the case, please remove it because:

    Features from other markdown flavors must have an option that turns them on or off. If the feature is common enough across multiple versions of markdown, it may be turned on by default. This shall be decided by the community when the merge request is done.

@fmoralesc

This comment has been minimized.

Contributor

fmoralesc commented Jun 2, 2014

  1. I added abort because vimgrep might fail and in that case the function should return inmediately. I could catch the exception and return if that's preferred.
    2), 3), 4) OK
  2. This might be a difference between vim-pantondoc's and vim-markdown version. I massage the output further, so I don't have that problem there.
  3. That was something out of vim-pantondoc too. I forgot about it, and simply added the vimgrep call as is used there.
@cirosantilli

This comment has been minimized.

Collaborator

cirosantilli commented Jun 2, 2014

  1. I prefer exceptions as they make it clear what part of the function can fail. In what case can vimgrep fail?

  2. Please test with this repository so we can reproduce each other.

@fmoralesc

This comment has been minimized.

Contributor

fmoralesc commented Jun 2, 2014

  1. When there are no headers in the document (which would obviously fail to happen in the test document linked ;)).
  2. Sure.
@cirosantilli

This comment has been minimized.

Collaborator

cirosantilli commented Jun 2, 2014

When there are no matches, I propose we capture the error and open an empty quickfix. Please keep this change for another PR as it is not related to the addition of Setex.

@fmoralesc

This comment has been minimized.

Contributor

fmoralesc commented Jun 2, 2014

On 5) I've found this is not because of the text processing, but because the horizontal lines are being matched by the vimgrep regex. I'll fix it.

@fmoralesc

This comment has been minimized.

Contributor

fmoralesc commented Jun 2, 2014

On the previous comment: why should the user want to open an empty quickfix, though? I think it's better to fail silently (like command line apps who don't emit output when they don't have stuff to do) or maybe message the user.

@cirosantilli

This comment has been minimized.

Collaborator

cirosantilli commented Jun 2, 2014

Agree for a "No headers." message.

@fmoralesc

This comment has been minimized.

Contributor

fmoralesc commented Jun 2, 2014

All discussed points covered in 2c06d3d.

@cirosantilli cirosantilli merged commit 2c06d3d into plasticboy:master Jun 3, 2014

@cirosantilli

This comment has been minimized.

Collaborator

cirosantilli commented Jun 3, 2014

There were three issues which I resolved myself in 2a58c2f:

  • one hard tab
  • multiple spaces after the hash sign: #__a
  • trailing hash signs: # a #

Please check the tests more carefully next time ; )

And thanks for the contrib!

@fmoralesc

This comment has been minimized.

Contributor

fmoralesc commented Jun 3, 2014

For the carelessness, I'll blame it on being busy :p

Noticed the problem with the hashes myself. I'll grab the fixes for vim-pantondoc, if you don't mind ;)

Check vim-pantondoc (soon to be vim-pandoc) and vim-pandoc-syntax when you have the time, you might find useful stuff to integrate in this plugin.

@cirosantilli

This comment has been minimized.

Collaborator

cirosantilli commented Jun 3, 2014

No problem.

Feel free to take whatever you like from here, I'll have a look too. Do you have any particular features in mind which vim-pandoc has and this lacks, specially not pandoc specific ones?

Note that this project aims at covering extensions such as Pandoc's. We would be really glad if you contributed them here directly so we don't duplicate work. It's already hard enough to get one vim-markdown right: might be more productive to work in a single project.

Or are there very different design principles which turn you away from this repository?

I'll review and incorporate whatever PRs you submit.

@fmoralesc

This comment has been minimized.

Contributor

fmoralesc commented Jun 3, 2014

Be aware I already took all ideas I found useful ;) (Mainly, navigation, which I extended, and the Toc functionality).

We have some design differences.

vim-pantondoc is not a filetype plugin per se (at least, not one in the usual sense). For one, it is markup syntax agnostic. We provide a syntax file in vim-pandoc-syntax, but the user can decide not to use it for all markdown files (so, for example, it can coexist with this plugin's). The pandoc functionality can be attached to other markups, and we currently support textile files too. If it makes sense to you, vim-pantondoc is similar to an emacs minor mode, except when the filetype is set to pandoc.

vim-pandoc-syntax aims to provide support for pandoc markdown only, which is quite the beast. When I started using pandoc, I found this plugin's and tpope's syntaxes not complete enough, so I chose to use vim-pandoc, which someone else had made and dsanson had turned into a bundle. I started working on the syntax, and then took over to add pandoc execution and bibliographies support (which is totally out of scope for you, I guess ;)).

Onwards, I guess we could gather under a vim-markdown organization (like dsanson and I did when we created the vim-pandoc org), so we don't duplicate work. Anyway, if you need assistance or want to discuss this further, just get in touch. My email is hel dot sheep at gmail dot com.

@fmoralesc

This comment has been minimized.

Contributor

fmoralesc commented Jun 3, 2014

Forgot to mention: Please take in account dsanson's original idea was to provide a plugin that bundled everything someone using pandoc might want to have in vim, and I've tried to follow it as far as possible. This is the reason vim-pandoc and vim-pantondoc do so much, even when other plugins might provide alternatives (like the Toc functionality, which Voom could provide, or our keyboard stuff...). The original vim-pandoc included configuration for external plugins too, but that's where I draw the line. We still maintain that, though.

@fmoralesc fmoralesc deleted the fmoralesc:patch-1 branch Jun 3, 2014

@cirosantilli

This comment has been minimized.

Collaborator

cirosantilli commented Jul 2, 2014

@fmoralesc I think this caused: #100

@fmoralesc

This comment has been minimized.

Contributor

fmoralesc commented Jul 4, 2014

I'll take a look into it. I'm on the country visiting my father, so it
might take a while.

On Wed, Jul 2, 2014 at 5:19 AM, Ciro Santilli notifications@github.com
wrote:

@fmoralesc https://github.com/fmoralesc I think this caused: #100
#100


Reply to this email directly or view it on GitHub
#80 (comment)
.

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