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

Actually Print EditorConfig Warning Messages #51

Closed
samontea opened this issue Jun 6, 2017 · 7 comments · Fixed by #52
Closed

Actually Print EditorConfig Warning Messages #51

samontea opened this issue Jun 6, 2017 · 7 comments · Fixed by #52

Comments

@samontea
Copy link
Contributor

samontea commented Jun 6, 2017

Editorconfig overrides existing language code style settings in favor of project defined style guides in the form of .editorconfig file. This package does require an external dependency for the emacs package. We need to indicate to the user that they need to install this dep.

@samontea
Copy link
Contributor Author

samontea commented Jun 6, 2017

None of our existing packages require an external dependency. I felt like this merited more discussion about how to deal with this. cc: @rye @cg505

@rye rye changed the title Support for editorconfig. Support for editorconfig Jun 6, 2017
@rye
Copy link
Member

rye commented Jun 6, 2017

I agree. EditorConfig is a good example of something that should exist in our configuration, because it's a great feature from our previous configurations that worked very well in contributing to projects that use an esoteric code style e.g. soft tabs or hard tabs.

The external dependency for EditorConfig is a program (EditorConfig C Core) that can be run on the host machine. I agree that we should figure out how to deal with cases like these; I'd imagine we don't want to install external dependencies for our users, but it might make sense to point them to a Wiki entry—through a minibuffer warning or something like that—for the different edge cases where the host environment doesn't have everything available.

@rye rye changed the title Support for editorconfig Support for EditorConfig Jun 6, 2017
@cg505
Copy link
Contributor

cg505 commented Jun 6, 2017

#10 #18
I straight up thought we already had this working

@rye
Copy link
Member

rye commented Jun 6, 2017

#18, too.

As with most PR's, this is low-impact and adds functionality so it doesn't matter too much if it's broken.

I guess this is broken. We tried to add it before but it seems like it's not working.

@samontea
Copy link
Contributor Author

samontea commented Jun 6, 2017

okay so I guess the only remaining issue is indicating to the user that they should install the external dep. I updated the issue to reflect this.

@rye
Copy link
Member

rye commented Jun 6, 2017

Yeah the lwarn doesn't actually print anything out because its level is set to debug.

@rye rye changed the title Support for EditorConfig Actually Print EditorConfig Warning Messages Jun 6, 2017
rye added a commit that referenced this issue Jun 6, 2017
This is just a simple fix for #51.  I'm not sure if writing a message
is the right way to do this, and if we wanted to use `lwarn`, we
could, just changing `:debug` to `:warning`.  I think this follows our
pattern for previous things.

[GitHub] This commit resolves #51.
@rye
Copy link
Member

rye commented Jun 6, 2017

We can open another issue for hypotheticals.

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

Successfully merging a pull request may close this issue.

3 participants