pearofducks / ansible-vim

A vim plugin for syntax highlighting Ansible's common filetypes
MIT License
800 stars 98 forks source link

Added several keywords #7

Closed dvarrazzo closed 8 years ago

dvarrazzo commented 8 years ago

Keywords added: always_run changed_when failed_when no_log args vars delegate_to ignore_errors

Removed 'ignore' as keyword: it is not really special enough (even more in ansible 2.0)

pearofducks commented 8 years ago

Most of these are good additions, but I have some thoughts on a couple. Feel free to respond with your logic! (I assume you mean 'include' instead of 'ignore' for keywords?)

dvarrazzo commented 8 years ago

Hi,

my rationale was basically that everything in each ansible tasks file would have been highlighted, except name and the command itself (why special-casing name is actually not something I can really rationalize: mostly because the idiomatic pattern of a task is:

- name: this is my task
  whatevs: this is the command, there are many different ones
  sudo: true
  when: sometimes
  notify: someone
  ...

I think your pattern was to see the first two lines of a certain colour and the other modifiers as something else. I think you just missed several ansible features which we used instead in a 40-roles-410-tasks-strong deployment system of ours).

Include doesn't sound that different from other tasks: you can have:

- name: set up the sync user
  include: user.yml
  vars:
    mode: sync
    username: "{{ssh_user_sync}}"

which would follow the same colour convention of the other commands in the same playbook tasks file (note: this is from a tasks file, not from a playbook).

You can also see in this example why to consider vars a keyboard: with this idiom it follows the pattern of the other actions; not using it makes include brittle (see https://github.com/ansible/ansible/pull/14214 for my test case: using vars instead of the "flat" list of variables makes include less prone to names conflicts).

If you don't think the same about include (you are perfectly entitled to), I'd say you should probably consider 'meta' a keyword too.

dvarrazzo commented 8 years ago

(please note that I don't know other uses of vars: I only added it as include companion actually)

pearofducks commented 8 years ago

Your rationale for these makes sense, I'd like to add some of these as switchable highlights to keep things not-too-opinionated. I'll merge this once I get those switches set up for an immediate commit afterward!

Thanks for everything here :)

pearofducks commented 8 years ago

Merged now and added the flag (documenting soon!). I do not agree with the removal of include - via flag or no - and feel that's a bit too niche of a change to be making at this time. I can see how it's a bit different than the other flags we highlight though, so I'll be looking into it in the future.

Thanks again for the PR!