Integrate YCMD JSON settings

[ChoppinBlockParty]

Add ycmd-settings-json-filepath - path to YCMD settings JSON file.
YCMD accepts a json file where the server settings are set, including
a one-time HMAC. The file is deleted after the server starts.
As for now, emacs-ycmd was just creating a temporary file from
some hard-coded/customizable values.
Since those settings have to direct relation to emacs-ycmd, rather
only to YCMD, it is good idea to keep it this way and just feed the
file adding an HMAC value.

That change will read the JSON file, insert HMAC and pass it to YCMD.
Please, test it, feedback is appreciated, I will mainly test it on
C/C++ and Go.

That is supposed to solve #472, #479.

I copied ycmd/default_settings.json to my .emacs. and use it

ycmd-settings-json-filepath (concat user-emacs-directory "ycmd_default_setting.json")

[ptrv]

ycmd-extra-conf-handler is used also in ycmd--handle-extra-conf-exception so it needs to be saved from the json.

[ptrv]

This makes also a couple of defcustoms redundant and need to be removed

[ChoppinBlockParty]

ycmd-extra-conf-handler is used also in ycmd--handle-extra-conf-exception so it needs to be saved from the json.

I am not sure about that, because that is just how emacs-ycmd displays exceptions from ycmd. ycmd's option to confirm config is a little bit different, it is about does the server send this exceptions or not.

[ptrv]

Actually you are right. ycmd-extra-conf-handler also has to be removed, also from the check in ycmd--handle-extra-conf-exception

[ChoppinBlockParty]

I do not have a strong opinion, but I find it ok to keep ycmd-extra-conf-handler even, though it does overlap in behavior with the ycmd's option, it still has some neat behavior control over how emacs handles the exceptions, that someone might like. Ofc, it is not useful if the exceptions are totally disabled from the JSON settings, but I think it is fine.

[ptrv]

For this it's better to use the f.el library and this (f-join (f-dirname (f-this-file)) "ycmd_default_settings.json") because it handles also the cases when the file is loaded with eval-buffer, rather than loaded from the init file.
f.el is already used in company-ycmd and needs only to be added to the required packages to the header.

[ptrv]

Maybe we should keep the way we load settings and add loading from a json file as additional option, because this adds an extra step to setting up emacs-ycmd (which is already a pain point). Or we add a default json file to the repo. The setting should work out of the box.

[ChoppinBlockParty]

Actually, I intended to add the default settings file, but forgot to commit it. Thank you for the review. I will clean up the stuff you mentioned earlier.

[abingham]

First of all, thanks for putting this together. I think this is a big step forward for emacs-ycmd.

I do have some concerns about the design, though. My primary concern is that we've moved a lot of stuff that was under programmatic control and put it into a JSON file. For example, users used to be able to set ycmd-gocode-binary-path, and it would be used by ycmd; they could do this entirely within emacs lisp without having to really know much about ycmd itself. I considered this a win. Now, in order to do any sort of ycmd customization, they have to create an entire ycmd conf file.

As I mentioned in my comments in #479, I was hoping for a system that augmented the existing implementation rather than replacing it. I imagined that we would keep the original emacs-lisp-based system intact (i.e. ycmd--options-contents or something equivalent), but we would also let users specify a json config file. The contents of that file would be merged in to the configuration generated by ycmd--options-contents.

As I see it, this approach has several benefits:

1. It doesn't require an extra file (i.e. the default settings json file)
2. It keeps a lot of things under emacs-lisp control. In principle, in fact, we could put the entire configuration under emacs-lisp control if we chose to.
3. It lets users use an external ycmd config file to modify only the parts they need to modify. With this PR, they have to either replace all or none of the default config.

It's possible that the merging I mention is hard or impossible, in which case I'm happy to be corrected. But am I missing some benefits of the approach taken in this PR?

[ChoppinBlockParty]

The advantage of this is to not duplicate the functionality that is already done in ycmd. At the beginning there was no ycmd, there was ycm - vim plugin, all options of the server were in vim's config. Now, there is ycmd and they moved the options from vim to ycmd json file, likewise emacs-ycmd should not keep server options inside itself for many reasons.
Of course, I see your concerns that the users who set up the options in emacs might get disappointed, however, that cleans up code removing duplicated functionality making the project less dependent on ycmd options. Though in reality I think 99% or more do not know anything about this options and do not touch them at all (we have provided a default ycmd json to the project, so this PR would not break anything for them).

[abingham]

emacs-ycmd should not keep server options inside itself for many reasons.

What reasons? I'm asking in all seriousness, because it's entirely possible that I'm missing an important point.

As far as I can see, though, with your approach we still have to maintain and ship a JSON file with all of the ycmd configuration options in there, so we haven't gained any freedom from needing to know that information. Given that we have to know what goes in that file, I don't see how putting it in a JSON file is better than keeping it in emacs-lisp. On other hand, I think there are practical benefits to keeping it in emacs-lisp (though perhaps minor ones, as you say).

making the project less dependent on ycmd options.

As long as we have to maintain a JSON file containing all of the options with their default values, we are still dependent on them. Now, if we could get the ycmd server to generate a default configuration for us on demand (i.e. so that we didn't have to know anything about it), that would be a different story. As it is, though, we've just moved the location of a dependency from one place to another, while degrading actual and potential functionality.

I guess I'm struggling to see how the new JSON file you're proposing is any better than keeping identical information in emacs-lisp. Either we need to know that information or we don't.

[ChoppinBlockParty]

As long as we have to maintain a JSON file containing all of the options with their default values, we are still dependent on them.

ycmd ships this default file. In this revision we added it only to avoid breaking changes, or reduce them to minimum, with the current version of emacs-ycmd. In the future we might post a fat comment in the readme, to draw people attention to switch to ycmd config. Also it is possible to provide an option to set a path to ycmd. This path will be used for two things: json config and ycmd command, with this you do not need to ship the options (and the side improvement would be to set a default command prefix to python3 -u, that will make it easier to start with emacs-ycmd).

I guess I'm struggling to see how the new JSON file you're proposing is any better than keeping identical information in emacs-lisp. Either we need to know that information or we don't.

If you take into account what I said above, this means that you do not need to know this information. As I said in the comments above it has no relevance to emacs-ycmd itself, it is only relevant to ycmd.

[abingham]

In the future we might post a fat comment in the readme, to draw people attention to switch to ycmd config.

I guess this is where we differ. I'm strongly opposed to requiring people to have a ycmd config, esp. as we've already demonstrated that we can provide one for them. I think it's part of our job to make using ycmd as painless as possible, while at the same time giving them the flexibility to configure it as much as they want to. A lot of users - myself included - will never have a need to know about these configuration files, and I think that we're doing them a service by not forcing them to.

As I said in the comments above it has no relevance to emacs-ycmd itself, it is only relevant to ycmd.

I'm sorry, but this is nonsense. emacs-ycmd only exists to make ycmd accessible to emacs users. To say that the details of ycmd aren't "relevant" to emacs-ycmd is some sort of software engineering fundamentalism.

[ChoppinBlockParty]

this is nonsense

software engineering fundamentalism.

Relax, it is just 50 line pull request, no reasons to get that emotional.

[abingham]

You're right, and I apologize. I was in a pretty bad mood when I wrote that, and you got the short end of it. My technical objections to the direction you're taking still stand, though. We shouldn't worry too much about dependencies on ycmd; depending on it is the whole point of emacs-ycmd, in a sense. And I want to keep the barrier of entry for users as low as possible, so requiring them to create a JSON config just to get completion for Python or something feels like a non-starter. The scenario I typically consider is someone who wants to try emacs, so they've downloaded spacemacs. They're already in an unfamiliar and perhaps uncomfortable space, and I don't want to increase their chances of abandoning the endeavour because emacs-ycmd adds too much cognitive load.

…

On Fri, Oct 19, 2018 at 4:33 PM Yuki ***@***.***> wrote: this is nonsense software engineering fundamentalism. Relax, it is just 50 line pull request. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#485 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAE1eHRJivJInMyxmR05vQOyviMdrIqUks5umeLTgaJpZM4XtRD2> .