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

Migrate from juvenn/mustache.vim and nono/vim-handlebars #1

Closed
4 tasks done
juvenn opened this issue Oct 24, 2013 · 14 comments
Closed
4 tasks done

Migrate from juvenn/mustache.vim and nono/vim-handlebars #1

juvenn opened this issue Oct 24, 2013 · 14 comments
Assignees

Comments

@juvenn
Copy link
Contributor

juvenn commented Oct 24, 2013

Now the repo is setup, here's the plan to migrate the code and preserve commit history:

  • Push all the latest code from juvenn/mustache.vim and preserving all commit history @juvenn
  • Bring changes from nono/vim-handlebars and try to preserve everything @nono have @bruno- @nono
  • Update README at mustache/vim-mode @juvenn
  • Deprecate juvenn/mustache.vim and nono/vim-handlebars in the README, and pointing to mustache/vim-mode @nono @juvenn

Please add your ideas!

@ghost ghost assigned juvenn Oct 24, 2013
@juvenn
Copy link
Contributor Author

juvenn commented Oct 24, 2013

Just initially setup README, would much like to have better word from you.

@juvenn
Copy link
Contributor Author

juvenn commented Oct 25, 2013

Deprecated juvenn/mustache.vim.

@bruno- could you please bring in changes from nono/vim-handlebars?

Thanks!

@bruno-
Copy link
Member

bruno- commented Oct 25, 2013

Hey @juvenn ,
sure will! Please give me a couple of days because of other work.

@nono
Copy link
Member

nono commented Oct 26, 2013

Hi, I've pushed the changes from vim-handlebars. You should review them as some changes are invasive.

@juvenn
Copy link
Contributor Author

juvenn commented Oct 27, 2013

Thanks @nono. And no worries @bruno-, better to make it a happy plugin than a hurry one.

@bruno-
Copy link
Member

bruno- commented Oct 27, 2013

Hey guys, I've reviewed the updates, and here's what I noticed:

  • we have a highlight definition of a non-existing highlight group on lines 65 and 66 of the syntax file link to the line here.
  • mustacheInside region does not have highlight definition. I think this is related to previous bullet - so that we should just change mustacheVariable highlight definition on line 65 with mustacheInside.
  • by using this syntax highlighting, mustache / handlebars tags aren't detected well when inside the <title> tag of the example file.
<title>
  {{title}}
</title>
# title text inside the tags has highlighting groups "htmlTitle htmlHead", but it should be "mustacheInside"

I accidentally found a solution to this (following bullet).

  • I'm not sure why many regions/matches have containedin=@htmlMustacheContainer. Everything works fine when @htmlMustacheContainer is removed. It even fixes the above issue with title.

There may be more things that I haven't noticed.

Would you agree we should submit a pull request if there is a change proposal, instead of committing directly to master? That way, we all have time to review the changes and give suggestions before they are in master. What do you think?

@bruno-
Copy link
Member

bruno- commented Oct 27, 2013

Hey guys, here's a suggestion what we should do before asking Bram Moolenaar to merge our plugin to vim:

  • ask an experienced vim plugin author to review our plugin and give suggestions.

Here's some people that are good with vimscript:

  • Drew Neil, maker of vimcasts.com or
  • Tim Pope - author of several very popular plugins or
  • Steve Losh - author of this great book about vimscript

@nono
Copy link
Member

nono commented Oct 28, 2013

Hi,

mustacheInside region does not have highlight definition

Yes, it cas my intention. It helps for building other groups but I don't think it deserve its own coloration.

For the bug on <title> and @htmlMustacheContainer, it's weird. I've added @htmlMustacheContainer to fix this bug (ie bad coloration without them and it's OK with them). But you seems to have the opposite behaviour... I don't know what to do.

Would you agree we should submit a pull request if there is a change proposal, instead of committing directly to master?

OK for me.

ask an experienced vim plugin author to review our plugin and give suggestions.

Great idea!

@juvenn
Copy link
Contributor Author

juvenn commented Oct 28, 2013

Would you agree we should submit a pull request if there is a change proposal, instead of committing directly to master?

Agree, we should do that.

ask an experienced vim plugin author to review our plugin and give suggestions.

Great idea! Please cc me if you email them.

@bruno- , would you mind creating another issue where we follow about asking for advice and making it into vim? So we can close this issue as long as we make the code clean.

@bruno-
Copy link
Member

bruno- commented Oct 29, 2013

Hey @juvenn , I've opened a separate issue/task #4 for code review. We can delete related post from this issue.

@bruno-
Copy link
Member

bruno- commented Oct 29, 2013

Hi @nono ,
about that issue around the <title> tag and @htmlMustacheContainer - I think we should not pay attention to coloration of the elements but to which syntax groups elements have.

Coloration is based on colorscheme and varies wildly from user to user. We should not depend on this for inspection.
On the other hand - syntax groups and matches are consistent. This is what we define in syntax file, and this should be checked when inspecting.

Here is how to check for syntax groups for any character on the page:

  • place cursor on element where you wanna know syntax groups
  • execute this in vim command line echo reverse(map(synstack(line('.'), col('.')), 'synIDattr(v:val,"name")')). This will return an array of syntax groups for the character under cursor.

Simpler alternative to this is to use vim-scriptease plugin. Among other things, it has a zS command that returns similar result to this vimscript line above. (I use this plugin and it's very good).

For the problem we have, @nono, can you please try to debug the issue with the <title> tag?
Here's what I'm getting:

  • for curly braces {{ inside the <title> tag I get the following syntax groups: "mustacheHandlebars htmlTitle htmlHead". This seems ok.
  • for the text inside the curly braces within <title> tag, I get syntax groups: "htmlTitle htmlHead". This is not ok, because this text should be a "mustacheInside" also.

Can you please let me know what are your results?

@nono
Copy link
Member

nono commented Oct 29, 2013

Hi @bruno-

I have the same results when using master. If I remove the @htmlMustacheContainer from containedin rules, I get "htmlTitle htmlHead" on {{ as well as on the text inside the curly braces (ie mustacheHandlebars is missing on the first case).

I think that even the first case is not correct in master, we should have mustacheInside for the curly braces {{. But I don't see from the syntax files what makes mustacheInside not working inside head/title. DO you have an idea?

@juvenn
Copy link
Contributor Author

juvenn commented Oct 29, 2013

Hey @juvenn , I've opened a separate issue/task #4 for code review. We can delete related post from this issue.

Yes, seeing that issue. Though it's fine leaving those comments here, no need to delete them.

We'll close this issue, as long as you've figured out the @htmlMustacheContainer issue.

@bruno-
Copy link
Member

bruno- commented Feb 23, 2014

Hey guys, this has been open for a long time. We did all the main steps in moving to the new repository so I'm going to close this.

@bruno- bruno- closed this as completed Feb 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants