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

Assess status of EditorConfig support #54

Closed
cg505 opened this issue Jun 6, 2017 · 13 comments
Closed

Assess status of EditorConfig support #54

cg505 opened this issue Jun 6, 2017 · 13 comments

Comments

@cg505
Copy link
Contributor

cg505 commented Jun 6, 2017

In #51 we concluded we were not correctly warning the user when they did not have have the EditorConfig executable installed. This was fixed in PR #52. However, I think this needs reevaluated. The main reason is that the emacs package for EditorConfig has fallback parsing support that doesn't depend on the binary and seems to be fully functional. If we have the binary, we should use it, but it doesn't seem to make sense to me to warn the user every time they start up if they don't have a binary that they don't even really need.

Other relevant issues/PRs: #10 #18.

@rye rye added this to Review-Ready in Emacs Configuration Jun 6, 2017
@rye rye moved this from Review-Ready to Backlog in Emacs Configuration Jun 6, 2017
@rye rye moved this from Backlog to Ready in Emacs Configuration Jun 7, 2017
@rye rye moved this from Ready to In Progress in Emacs Configuration Jun 7, 2017
@rye rye self-assigned this Jun 7, 2017
@rye
Copy link
Member

rye commented Jun 7, 2017

For reference, some links:

I remember somewhat vaguely my first pass at this, both with #10 and subsequently #18, so I will take responsibility for this one. (Self-assign complete.)

It appears that EditorConfig gracefully falls back to the packaged EmacsLisp implementation of the EditorConfig core in the event that the EditorConfig C Core Executable is not found. The EmacsLisp implementation is meant to be a fallback, replacing most of the important EditorConfig functionality—getting a configuration hash from a .editorconfig file if one exists; applying said hash—with its own implementation in EmacsLisp.

I posit that the Emacs EditorConfig plugin's built-in EditorConfig support is "good enough," because I did not know that I did not have the EditorConfig C Core installed until I saw #51 and tried to uninstall it to no avail. However, I haven't actually used it yet on any projects, so that would explain why. One way or the other, if the EditorConfig C Core is updated, the Emacs Plugin will be what determines whether support for a new feature is turned on—even if the user upgrades EditorConfig C Core to a new version where support for a new feature exists, the plugin still has to catch that and use it, and it's likely that that implementation would take place at the same time as a change to the EmacsLisp implementation.

So, @cg505's assertion that

[...] it doesn't seem to make sense to me to warn the user every time they start up if they don't have a binary that they don't even really need.

is a correct one. I think that if we revert the changes made in #52 to make our configuration use lwarn to produce a :debug-level message, that is sufficient—an adept user will install EditorConfig C Core.

If we want, we could potentially keep this as a message and add a suppression variable, but I don't really like the idea of forcing users to do this manually, nor putting it in a personal configuration, nor making some kind of self-overwriting code. So, I'm curious what sounds good to @cg505 or @samontea. Either way, it's sufficient not to warn the user more than once, so we can revert the changes made by #52.

Also, if there's a problem with our EditorConfig configuration, you can apparently "always use [EditorConfig's EmacsLisp implementation]" by adding

(setf editorconfig-get-properties-function
      #'editorconfig-core-get-properties-hash)

to your machine-local configuration.

@rye
Copy link
Member

rye commented Jun 7, 2017

I propose that our solution be to first revert the changes introduced in #52, making the warning silent again, and then to discuss this and figure out how to warn the user.

@cg505
Copy link
Contributor Author

cg505 commented Jun 8, 2017

sounds fine

@cg505
Copy link
Contributor Author

cg505 commented Jul 16, 2017

Is this still in progress?

@rye
Copy link
Member

rye commented Jul 16, 2017

No.

@rye rye moved this from In Progress to Ready in Emacs Configuration Jul 16, 2017
@rye
Copy link
Member

rye commented Jul 30, 2017

@cg505 this still hasn't been touched. As I suggested, we should make the warning silent again and discuss how to warn the user. Is there some easy way to do all of this on the first startup?

@cg505
Copy link
Contributor Author

cg505 commented Jul 31, 2017

Some way to warn the user on startup? Honestly a message is probably the easiest without being too intrusive.

@rye
Copy link
Member

rye commented Jul 31, 2017

I think the point was made that warning the user every time they start up was annoying? Should we only warn on the first start up or what?

@cg505
Copy link
Contributor Author

cg505 commented Jul 31, 2017

I mean if you send a message during startup it's gonna get buried anyway, so you'd have to go to the *Messages* buffer to see it.

@rye
Copy link
Member

rye commented Jul 31, 2017

So... the changes introduced in #52 were all that was needed?

@rye
Copy link
Member

rye commented Jul 31, 2017

In that case this issue should be closed.

@cg505
Copy link
Contributor Author

cg505 commented Jul 31, 2017

I mean... yeah I don't have anything better.

@rye
Copy link
Member

rye commented Jul 31, 2017

Okay. I'll close this issue then.

@rye rye closed this as completed Jul 31, 2017
@rye rye moved this from Ready for Work to Done in Emacs Configuration Jul 31, 2017
@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