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

EditorConfig #18

Merged
merged 10 commits into from
Feb 3, 2017
Merged

EditorConfig #18

merged 10 commits into from
Feb 3, 2017

Conversation

rye
Copy link
Member

@rye rye commented Feb 1, 2017

This probably sucks and isn't exactly what we want, especially with the core warning, but it does implement basic support for editorconfig-mode. I have tested it and it does effectively change indentation style.

Conflicts:
	.emacs.d/lisp/package/dependencies.el
   - Fixed by keeping all packages.
This is the simplest possible configuration for everything.
This new function is run as an EditorConfig custom hook, so it only
takes place in situations where EditorConfig can be loaded.
This might suck.
@rye rye self-assigned this Feb 1, 2017
@rye rye requested a review from cg505 February 1, 2017 20:47
Copy link
Contributor

@cg505 cg505 left a comment

Choose a reason for hiding this comment

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

I'm not at machine right now, so I haven't really tested this yet. I'll play around with it a bit this afternoon.

;; Enable EditorConfig mode
(editorconfig-mode +1)

(defun kotct/editorconfig--disable-smart-tabs-if-indent-tabs-mode-disabled (&optional hash)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please can we shorten some of the names? This is a little excessive.That's what the docstring's for.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also what's the point of HASH?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have a suggestion for a better name? hash is a hash passed by EditorConfig when it runs the hooks. It contains the loaded properties from the EditorConfig file.

Copy link
Contributor

Choose a reason for hiding this comment

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

It'd go with something like kotct/editorconfig-maybe-disable-smart-tabs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also the hash thing sounds fine. Maybe mention that it's only there for compatibility and is ignored in the docstring.

nil
"If non-nil, warning on startup is suppressed.")

(defun kotct/editorconfig-check-for-core (&optional suppress)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's choose either -- or - and stick to it. Personally I'm in favor of -.

Copy link
Member Author

@rye rye Feb 3, 2017

Choose a reason for hiding this comment

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

Okay. - works. I generally use -- for situations when I don't want people calling the functions, so really everything here should be --.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. We haven't been using that convention in dot, but we could lol.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should. Anything that we don't want users to use (i.e. stuff that isn't (interactive)-ified) should be with double dashes, imo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the fact that it's not interactive enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

Aye, but we don't want the functions mixed in with the ones that are interactive when you're using something like an autocompleter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay.
I'm open to this if you want to go through and change everything on a branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

For now let's stick to -?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose.

Debug warning is suppressed if SUPPRESS is non-nil."

(if (not suppress)
(if (not (executable-find editorconfig-exec-path))
Copy link
Contributor

Choose a reason for hiding this comment

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

editorconfig-exec-path sounds like a path, not an executable name. If this is the case, we should be using something that checks if the file exists, not executable-find.

Copy link
Member Author

@rye rye Feb 3, 2017

Choose a reason for hiding this comment

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

No, it's an executable name. (i.e. editorconfig, no pathing)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yikes. Bad name. Is that the editorconfig package's fault then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but it makes sense. This is platform-agnostic, and uses Emacs' builtin which-like mechanisms to hunt down the executable. They should just use editorconfig-executable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep. Looks good.

We can switch back to double-dash once we have a specification we
actually want to implement.
@rye rye requested a review from cg505 February 3, 2017 20:04
@rye
Copy link
Member Author

rye commented Feb 3, 2017

@cg505 I'm going to go ahead and merge this. As with most PR's, this is low-impact and adds functionality so it doesn't matter too much if it's broken.

@rye rye merged commit faea445 into master Feb 3, 2017
@rye rye deleted the editor-config branch February 3, 2017 23:02
@rye rye modified the milestone: Version 0.0.0 Aug 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants