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

consult-flycheck #51

Closed
mpenet opened this issue Dec 10, 2020 · 11 comments
Closed

consult-flycheck #51

mpenet opened this issue Dec 10, 2020 · 11 comments

Comments

@mpenet
Copy link

mpenet commented Dec 10, 2020

This is just a suggestion:

Would you consider adding a consult-flycheck command that would cycle through the entries in flycheck-list-errors, with jump to selected file at error point?

@minad minad mentioned this issue Dec 10, 2020
20 tasks
@minad
Copy link
Owner

minad commented Dec 10, 2020

Yes, this would be great, combined with preview! I would be using something like this, I am sure. I added it to the wishlist for now, feel free to create a PR.

@minad minad closed this as completed Dec 10, 2020
@minad
Copy link
Owner

minad commented Dec 10, 2020

@mpenet Already implemented this. This is great, I will use this from now on instead of the flycheck buffer I think!

@mpenet
Copy link
Author

mpenet commented Dec 10, 2020

oh wow, you rock, thanks!

@minad
Copy link
Owner

minad commented Dec 10, 2020

Please test it thoroughly, that would be nice. The way I implemented this differs from the way counsel does it. If you jump back and forth between the minibuffer, the error line numbers will not be correct anymore in the minibuffer obviously, but the jumping locations will be updated. Ideally the line numbers should also be updated, but this is not so easy since it requires dynamic candidate support #10. If it turns out that the current implementation is not working so well, I can use a simpler implementation, which does not update the locations.

@purcell
Copy link

purcell commented Dec 10, 2020

Erm, this should be a separate consult-flycheck package with proper dependencies. It's not sustainable, or good practice, to dump every possible extension into consult.el.

@purcell
Copy link

purcell commented Dec 10, 2020

Like, this immediately wouldn't pass MELPA review if submitted now.

@purcell
Copy link

purcell commented Dec 10, 2020

See #53 for the fix.

@minad
Copy link
Owner

minad commented Dec 10, 2020

To be honest I am not super happy with this. There is no such "consult" with which you can jump to flycheck errors. There is no integration of components going on, consult-flycheck is just a single command relying on some internal helper functions but there is no public consult api between the two packages. I don't want to end up with too many packages. Is the problem that flycheck is a non-core component? I also integrate with selectrum here, do I also need a consult-selectrum integration package?

@purcell
Copy link

purcell commented Dec 11, 2020

I don't want to end up with too many packages.

Think of it as organisation of code in the repo. I don't think you'd end up with "too many" actual packages, because the only separate packages would be for integration with other optional packages like flycheck.

Is the problem that flycheck is a non-core component?

Yes.

I also integrate with selectrum here, do I also need a consult-selectrum integration package?

Yes, that would also be cleaner, and quite feasible, looking at the code. A consult-selectrum package would add an entry to consult-preview-mode-hook to enable/disable its advice.

Basically anywhere you call functions (or read/write vars) declared in a non-core package, you should think about how to break that out.

It's harder to write code that works like this, but the result is more idiomatic and extensible.

@minad
Copy link
Owner

minad commented Dec 11, 2020

Yes, that would also be cleaner, and quite feasible, looking at the code. A consult-selectrum package would add an entry to consult-preview-mode-hook to enable/disable its advice.

Well, I already took care that the code is reasonably robust. As I said, the preview part is the most fragile part.

Regarding the split I am probably rather doing the all or nothing thing. Splitting up everything cleanly or not doing the split at all.

But there is also the usability problem - people are already complaining they needed selectrum, consult, embark, marginalia and orderless for example. Now if we tell them well you also need the N^2 integration libraries...

@purcell
Copy link

purcell commented Dec 11, 2020

But there is also the usability problem - people are already complaining they needed selectrum, consult, embark, marginalia and orderless for example. Now if we tell them well you also need the N^2 integration libraries...

I think that's pretty trivial tbh. The integration packages are listed next to each other, and I don't think it's as bad as N^2 in practice.

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

No branches or pull requests

3 participants