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

remove indentation configuration, smart tabs customizations #134

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

samontea
Copy link
Contributor

@samontea samontea commented Jun 6, 2020

We're currently in a funny game where we're trying to set sane defaults for languages, but also respect editorconfig configurations. However, we end up failing to respect editorconfig configurations in certain circumstances.

I think we should instead try to switch to just using editorconfig to set our indentation configurations.

If we want we can create a default .editorconfig file with sane indentation defaults and symlink it to ~/.editorconfig to give the user sane defaults for most languages.

@samontea samontea requested review from rye and cg505 June 6, 2020 04:11
@samontea samontea force-pushed the remove-indentation-configuration branch from 1941911 to 02500dc Compare June 6, 2020 04:14
@samontea samontea force-pushed the remove-indentation-configuration branch from 4c17da0 to 087eb45 Compare June 6, 2020 06:55
Copy link
Member

@rye rye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd re-title this PR as "Remove per-language indentation configuration, smart tabs customizations", as this PR doesn't (yet) touch the defaults specified in indentation.el to delete our changed defaults. I think we should completely remove kotct/tab-variable-setters and all of its related machinery, relying on language-specific implementations (and editorconfig-mode) to keep things as they should be. I do think the default value for indent-tabs-mode is fine, but other things will need to go to prevent clobbering.

It's worth noting to the skeptics here that editorconfig already has a lot of support for a lot of languages, and probably does a better job of keeping track of variables. Its configuration certainly was a drop-in replacement for me with Ruby code. We would still need language support files for the setup of additional tooling for each language, but no longer would indentation machinery matter.

I think it might also be smart to emit a warning if an EditorConfig is not found. The sexp (editorconfig-core-get-nearest-editorconfig default-directory) will return the filename of the nearest .editorconfig file, but I think I'd also like a warning to show up if an EditorConfig exists but does not apply to the current file. That way, we can warn the user if they're using Emacs' defaults, which I think is a scary enough thing to warrant warnings. 😄

@rye
Copy link
Member

rye commented Jun 8, 2020

After a bit of fiddling around, I've found a couple of helpful things that we could use to better support EditorConfig:

  • (editorconfig-get-properties) is responsible for finding and parsing the nearest EditorConfig file's rules for the current buffer. It returns the results in a hashtable, which is somewhat spooky but quite efficient.
  • (hash-table-count <table>) gives us a count of the values in a hash table.
  • So, if we take (hash-table-count (editorconfig-get-properties)), we'll get the number of properties applying to the current buffer.
    • We could then warn the user if the number of properties applying to the current buffer is 0—then hide that message behind some kotct/warn-user-if-editorconfig-doesnt-apply defvar.

@samontea, would you mind if I mess around with this branch?

@samontea samontea changed the title remove indentation configuration, smart parens remove indentation configuration, smart tabs customization Jun 8, 2020
@samontea
Copy link
Contributor Author

samontea commented Jun 8, 2020

Yeah feel free. Those changes sound good to me.

@samontea samontea changed the title remove indentation configuration, smart tabs customization remove indentation configuration, smart tabs customizations Jun 8, 2020
rye added 5 commits June 9, 2020 11:47
Signed-off-by: Kristofer Rye <kristofer.rye@gmail.com>
Tested-by: Kristofer Rye <kristofer.rye@gmail.com>
As of yet, there is no way to inhibit this behavior.

Signed-off-by: Kristofer Rye <kristofer.rye@gmail.com>
Tested-by: Kristofer Rye <kristofer.rye@gmail.com>
Signed-off-by: Kristofer Rye <kristofer.rye@gmail.com>
Signed-off-by: Kristofer Rye <kristofer.rye@gmail.com>
Tested-by: Kristofer Rye <kristofer.rye@gmail.com>
Signed-off-by: Kristofer Rye <kristofer.rye@gmail.com>
Copy link
Contributor Author

@samontea samontea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your changes look good. i'd just like a little test coverage if it's trivial/doable.

.emacs.d/lisp/code/editorconfig-c.el Show resolved Hide resolved
Copy link
Member

@rye rye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that if we're moving the smart tabs customizations per-language, we should also remove smart tabs support as a whole (i.e. remove it from the list of kotct dependencies and remove our logic that interacts with it). Otherwise we're effectively just hobbling smart tabs for the languages we add support for.

(I'm fine with removing the cases where we explicitly enable indent-tabs-mode and instead requiring indent-tabs-mode to be turned on by editorconfig. smart-tabs-mode should be enabled when indent-tabs-mode is.)

rye added 5 commits July 24, 2020 14:26
Signed-off-by: Kristofer Rye <kristofer.rye@gmail.com>
Signed-off-by: Kristofer Rye <kristofer.rye@gmail.com>
Signed-off-by: Kristofer Rye <kristofer.rye@gmail.com>
Signed-off-by: Kristofer Rye <kristofer.rye@gmail.com>
Signed-off-by: Kristofer Rye <kristofer.rye@gmail.com>
@samontea samontea mentioned this pull request Aug 24, 2020
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

Successfully merging this pull request may close these issues.

None yet

2 participants