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

[RFC] Remove 'edcompatible' #1911

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@ghost

ghost commented Jan 29, 2015

refs #1902

I know I don't like the idea of this option, but what does everyone else think?

@ghost ghost referenced this pull request Jan 29, 2015

Closed

Remove 'edcompatible' #1902

@marvim marvim added the RFC label Jan 29, 2015

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Jan 31, 2015

Member

LOL, I had no idea this existed.

Member

justinmk commented Jan 31, 2015

LOL, I had no idea this existed.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 16, 2015

Any comments? 😀

ghost commented Feb 16, 2015

Any comments? 😀

@elmart

This comment has been minimized.

Show comment
Hide comment
@elmart

elmart Feb 16, 2015

Member

When removing options, we should always consider make them noops instead of removing them completely. This is, setting the option to any value would make no difference, but wouldn't error. Aim is making possible for vim and nvim to use the same .vimrc, with special casing through has('nvim') redued to a minimum.

Member

elmart commented Feb 16, 2015

When removing options, we should always consider make them noops instead of removing them completely. This is, setting the option to any value would make no difference, but wouldn't error. Aim is making possible for vim and nvim to use the same .vimrc, with special casing through has('nvim') redued to a minimum.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Feb 16, 2015

Member

@elmart Agreed, but in this case, ignoring set edcompatible would have broken behavior that would just be wrong (for any plugin or user expecting it). However set noedcompatible should be ignored, because it wouldn't cause any harm. This is what we did for set cp / set nocp.

Member

justinmk commented Feb 16, 2015

@elmart Agreed, but in this case, ignoring set edcompatible would have broken behavior that would just be wrong (for any plugin or user expecting it). However set noedcompatible should be ignored, because it wouldn't cause any harm. This is what we did for set cp / set nocp.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 16, 2015

@justinmk the behavior you described should now happen.

ghost commented Feb 16, 2015

@justinmk the behavior you described should now happen.

@elmart

This comment has been minimized.

Show comment
Hide comment
@elmart

elmart Feb 16, 2015

Member

👍

Member

elmart commented Feb 16, 2015

👍

@justinmk

View changes

Show outdated Hide outdated src/nvim/ex_cmds.c Outdated
@justinmk

View changes

Show outdated Hide outdated src/nvim/ex_cmds.c Outdated
else if ((int *)varp == &p_ed && p_ed == TRUE) {
p_ed = FALSE;
return e_unsupportedoption;
}

This comment has been minimized.

@justinmk

justinmk Feb 17, 2015

Member

This can just be an || block on line 4949. We might find the collection growing...

  if (((int *)varp == &p_cp && p_cp == TRUE)      // Prevent "set compatible"
      || ((int *)varp == &p_ed && p_ed == TRUE))  // Prevent "set edcompatible"
  {
    /* Ensure that compatible can not be enabled */
    p_cp = FALSE;
    return e_unsupportedoption;
  }
@justinmk

justinmk Feb 17, 2015

Member

This can just be an || block on line 4949. We might find the collection growing...

  if (((int *)varp == &p_cp && p_cp == TRUE)      // Prevent "set compatible"
      || ((int *)varp == &p_ed && p_ed == TRUE))  // Prevent "set edcompatible"
  {
    /* Ensure that compatible can not be enabled */
    p_cp = FALSE;
    return e_unsupportedoption;
  }

This comment has been minimized.

@ghost

ghost Feb 17, 2015

I did it that way so that when one variable is found to be TRUE, then you don't need to set all unsupported variables to FALSE (your example doesn't reset p_ed to FALSE). There might be a more elegant way to do that...

@ghost

ghost Feb 17, 2015

I did it that way so that when one variable is found to be TRUE, then you don't need to set all unsupported variables to FALSE (your example doesn't reset p_ed to FALSE). There might be a more elegant way to do that...

This comment has been minimized.

@justinmk

justinmk Feb 17, 2015

Member

yes, my mistake.

@justinmk

justinmk Feb 17, 2015

Member

yes, my mistake.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 17, 2015

Re-pushed with recommended changes.

ghost commented Feb 17, 2015

Re-pushed with recommended changes.

justinmk added a commit that referenced this pull request Feb 17, 2015

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Feb 17, 2015

Member

merged

Member

justinmk commented Feb 17, 2015

merged

@justinmk justinmk closed this Feb 17, 2015

@justinmk justinmk removed the RFC label Feb 17, 2015

@ghost ghost deleted the rm-edcompatible branch Feb 17, 2015

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost commented Feb 17, 2015

Thanks!

justinmk added a commit that referenced this pull request Feb 17, 2015

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