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

Make consult-flycheck a separate package with correct dependencies #53

Closed
wants to merge 1 commit into from

Conversation

purcell
Copy link

@purcell purcell commented Dec 10, 2020

I can take care of the MELPA side of this.

@minad
Copy link
Owner

minad commented Dec 11, 2020

@purcell I thought a while about this. I feel a bit pressed about this issue - I would like to have the package available in MELPA but at the same time I don't want to split up the package just yet. There are multiple reasons:

  • It is too early, I have to let the design settle a bit more. In particular if an alternative flymake command is added, I might want to replace the commands by a unified version which automatically uses the correct backend. Note that the command you are criticizing here is only a few hours old.
  • I am not excluding splitting up the package at some point, in particular if there are multiple related commands, e.g. consult-org-* commands, which should rather go to a consult-org.el and maybe even to a separate MELPA package.
  • There is a lot of precedent for what I am doing here. While it is not the cleanest solution, it is not technically wrong. See for example counsel-flycheck or projectile-ripgrep for commands which work only if the corresponding package is available. For a more recent example, there is lsp-mode-diagnostics.el which also integrates with either flycheck or flymake - this goes into the direction I have in mind.

I can assure you, there won't be an explosion in commands. The plan is certainly not to add commands for arbitrary obscure packages or external programs. Maybe in this case with the addition of consult-flycheck I did not evaluate all possibilities carefully enough, since I got too excited after I found that it works really well. But if we now split the package, I am much more committed to having this particular command in this form.

@purcell
Copy link
Author

purcell commented Dec 11, 2020

It is too early, I have to let the design settle a bit more. In particular if an alternative flymake command is added, I might want to replace the commands by a unified version which automatically uses the correct backend. Note that the command you are criticizing here is only a few hours old.

I'm just here to offer advice on helping the design to settle! I'd like to use this library for the long term, and part of the appeal is in it not ending up just like counsel. :-)

Such a unified flycheck/flymake command is inadvisable in my experience. Too much magic. There are already built-in idioms in Emacs for doing this. For example: bind consult-flycheck in flycheck-mode-map, and consult-flymake in flymake's keymap. Much better than ad-hoc magic code that tries to detect what's going on.

I am not excluding splitting up the package at some point, in particular if there are multiple related commands, e.g. consult-org-* commands, which should rather go to a consult-org.el and maybe even to a separate MELPA package.

Generally it's only the add-ons for optional packages which would need to go in separate packages, so things like consult-org and the flymake support would still belong in the core IMO.

There is a lot of precedent for what I am doing here. While it is not the cleanest solution, it is not technically wrong. See for example counsel-flycheck or projectile-ripgrep for commands which work only if the corresponding package is available. For a more recent example, there is lsp-mode-diagnostics.el which also integrates with either flycheck or flymake - this goes into the direction I have in mind.

Yes, those are sub-optimal too. Both projectile and counsel are examples of packages that have turned into big monolithic grab-bags of random functionality with workarounds for byte compilation. Detecting what's installed and what the user wants to use is a highly fraught endeavour, and such packages usually end up trying to do that in ways that make incorrect assumptions. Not to mention having to declare functions that may or may not actually exist in the external .el file being implicitly depended upon. On the other hand, splitting things up so that they have clear dependencies and byte-compile cleanly against actual loaded libraries is a much better approach. Early in a project is a great time to take steps to do things more cleanly.

@minad
Copy link
Owner

minad commented Dec 11, 2020

I have to think about it a bit more. I agree with the things you write, a good early design is important. I agree with avoiding the monolith thing, our whole design is centered around avoiding that.

But ending up with one command per package also does not feel so good.

I wonder about one thing - is it possible to split and still distribute as one package or is this impossible due to the way dependencies are specified per package? This is not possible and would not make much sense either?

Generally - would you recommend splitting up the library more (on the file level, not package level)? There are some core functions, e.g. the helpers and everything related to consult--read. Then there is preview infrastructure entangled with consult--read and the preview-mode (this is the most fragile part). Well, and then there all the user facing commands. Many elisps libraries I have seen are single file.

@purcell
Copy link
Author

purcell commented Dec 11, 2020

And look, my intention is not to waltz in here and gripe about anything. I've seen and reviewed literally thousands of Emacs packages in the last eight years, and I care about the ecosystem. Nobody is going to rage-yank consult out of MELPA over these points. But one thing I've learned is that new elisp authors learn from popular packages, and some of those packages have set some really bad examples, e.g. multiple cursors with its mc/ prefix. yasnippet fixed its prefix to be a good citizen. So I'm wary of taking certain popular packages as the "right" way to do things, and I try to do my best to encourage authors to spread good practices, so that code gets better across the whole ecosystem as people learn from each other.

@purcell
Copy link
Author

purcell commented Dec 11, 2020

I wonder about one thing - is it possible to split and still distribute as one package or is this impossible due to the way dependencies are specified per package? This is not possible and would not make much sense either?

Yes, you can have multiple files in one package, but only one set of dependencies per package. In this repo, you could have multiple .el files belonging to a consult package, and also several that each corresponded to consult-xxx packages. Sharing a directory means using :files specs in the MELPA recipes, to make sure that consult-flycheck isn't included in consult's contents, for example, but that's generally not too terrible.

Generally - would you recommend splitting up the library more (on the file level, not package level)? There are some core functions, e.g. the helpers and everything related to consult--read. Then there is preview infrastructure entangled with consult--read and the preview-mode (this is the most fragile part). Well, and then there all the user facing commands.
Many elisps libraries I have seen are single file.

No, my personal favoured approach is to avoid over-splitting. It often makes the code hard to follow. In the case of consult I wouldn't rush to split it up just for the sake of things, at least not at this stage. flycheck.el is enormous, for example. :-)

@purcell
Copy link
Author

purcell commented Dec 11, 2020

(I've seen lots of authors struggle to split big packages up into multiple files, btw. You wouldn't believe the messes I have seen in certain cases, with workarounds to avoid circular dependencies etc.)

@minad
Copy link
Owner

minad commented Dec 11, 2020

Ok, thanks.

The good thing is that consult is not a structurally complicated package. There are a few details but besides it is a flat thing - a bunch of commands.

I will look at this issue again in the next days and see how things can be resolved.

@purcell
Copy link
Author

purcell commented Dec 11, 2020

Yeah, let me know if I can help at all. Really appreciate your work here, and enjoying using it.

@manuel-uberti
Copy link

FWIW, I totally agree with @purcell on this. Yes, you get more packages and files to manage, and probably more work to set it all up cleanly, but in the long term consult core can be free of heavy external dependencies and extensibility would come easier.

To a certain degree, that's the beauty of consult already: it freed selectrum of non-core functionalities while opening possible integration with other completing-read facilities. Hence consult is already on the right track. :)

@s-kostyaev
Copy link

s-kostyaev commented Dec 11, 2020

Not quite sure if creating N^2 dependencies is the way to make things simpler. Why soft dependencies are evil? I have different experience, but outside the Emacs world.

@minad
Copy link
Owner

minad commented Dec 11, 2020

@purcell @manuel-uberti @s-kostyaev I can see both sides here and I have experience with both approaches. Doing the split up can ultimately lead to the cleaner design, as you may have seen I proposed the embark/consult marginalia extraction in order to get a clean boundary there. In this case I am divided, I am not just following through with the one approach all the time, doing some judgement call instead. But I will try out how everything looks if things are split up in a separate branch/PR. I will try to separate the selectrum-specific logic too, except for the consult--buffer-selectrum code. The selectrum-specific buffer function should ultimately go away - I just have not found the correct way yet on how to do it. @oantolin, @clemera and I are discussing options in #10.

@minad
Copy link
Owner

minad commented Dec 11, 2020

See #54 for the WIP

@minad
Copy link
Owner

minad commented Dec 11, 2020

Closed in favor of #54

@minad minad closed this Dec 11, 2020
@minad minad mentioned this pull request Dec 12, 2020
7 tasks
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.

4 participants