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

Does sly-cl-indent have to provide cl-indent? #92

Closed
tarsius opened this issue Apr 30, 2016 · 19 comments
Closed

Does sly-cl-indent have to provide cl-indent? #92

tarsius opened this issue Apr 30, 2016 · 19 comments

Comments

@tarsius
Copy link
Contributor

tarsius commented Apr 30, 2016

Maybe it does, e.g. if sly-cl-indent is considered to be a drop in replacement for cl-indent, then doing so might be a reasonable course of action. (But that would only be the case if using the sly variant doesn't mess with other packages that depend on cl-indent.)

If you don't want to stop providing that feature, that's fine. But I need to know so that I can write down the reason for the manual patch to the database needed to prevent the issue this causes for users of epkg.el+borg.el. (A package which requires cl-indent will then depend on slime, currently the dependency is undefined, either slime or sly.

@tarsius
Copy link
Contributor Author

tarsius commented Apr 13, 2017

By the way cl-indent is now part of Emacs (as of 25.1) and is also available from GNU Elpa.

@joaotavora
Copy link
Owner

I have to take a better look at this, I would really love to ditch sly-cl-indent altogether...

@tarsius
Copy link
Contributor Author

tarsius commented Aug 17, 2017

Friendly ping!

@tarsius
Copy link
Contributor Author

tarsius commented Aug 17, 2017

I would really love to ditch sly-cl-indent altogether

That sounds like a good plan.

@joaotavora
Copy link
Owner

That sounds like a good plan.

I've had a better look and that is very hard. Only sly-cl-indent correctly manages that special loop indentation that I and many lispers can't live without. It appears cl-indent.el used to do this, but it doesn't anymore. I need to investigate this further. There is a way out, but it's basically throwing in the towel and probably also hard: Rename every symbol in sly-cl-indent.el that conflicts with cl-indent.el, and stop providing cl-indent

@tarsius
Copy link
Contributor Author

tarsius commented Sep 14, 2017

Thanks for looking into this. I hope you can find a suitable and not too laborious solution, but I will try not to bug you about it anymore ;-)

joaotavora added a commit that referenced this issue Sep 14, 2017
* contrib/sly-stickers.el (sly-stickers--make-recording): A
recording description contains a recording-ctime field.
(sly-stickers--replay-mode-map): Fix typo.
(sly-stickers--replay-ignored-stickers)
(sly-stickers--replay-total, sly-stickers--replay-recording)
(sly-stickers--replay-old-total): Delete.
(sly-stickers--replay-data): Replace deleted variables.
(sly-stickers-replay-refresh, sly-stickers--replay-refresh-1)
(sly-stickers-replay--current-sticker-interactive)
(sly-stickers-replay-toggle-ignore-sticker)
(sly-stickers-replay-toggle-ignore-zombies)
(sly-stickers-replay-reset-ignore-list): Rework for new data
structures.
(sly-stickers-replay): Query user about forgetting old recordings.
(sly-stickers-forget): Allows forgetting just a subset of recordings.

* contrib/slynk-stickers.lisp (:slynk-stickers): Export TOTAL-RECORDINGS
(*next-recording-id*): New variable.
(recording): Replace INDEX slot with ID
(initialize-instance): Don't set INDEX
(search-for-recording-1): Return recording and position. Docstring.
(describe-recording-for-emacs): Return ctime as second value in list.
(total-recordings): New slyfun.
(search-for-recording): Take IGNORED-IDS and IGNORE-ZOMBIES-P, not
IGNORE-SPEC. Return absolute index as second value.
(forget): Allow forgetting just a subset of recordings.
(find-recording-or-lose): Find recording by id, not index.
@joaotavora
Copy link
Owner

Yeah, i fear it's quite laborious.... Another option would be to get sly-cl-indent.el merged into emacs and have it replace cl-indent

These are its current authors according to a git log. Perhaps many of them have already signed copyright assignments or have otherwise contributed trivial patches. Didier Verna is apparently also there.

      2:Author: Deokhwan Kim <deokhwankim@gmail.com>
     11:Author: Luís Oliveira <loliveira@common-lisp.net>
     20:Author: Joao Tavora <joaotavora@gmail.com>
    114:Author: Samuel Freilich <sfreilich@google.com>
    120:Author: Stas Boukarev <sboukarev@common-lisp.net>
    127:Author: Nikodemus Siivola <nsiivola@common-lisp.net>

@joaotavora
Copy link
Owner

(By the way, the commit that references this has nothing to do with this, it's a headless commit meant to reference 91...)

@tarsius
Copy link
Contributor Author

tarsius commented Oct 14, 2018

I am closing my old issues.

@tarsius tarsius closed this as completed Oct 14, 2018
@joaotavora
Copy link
Owner

I am closing my old issues.

And I am reopening it, since it is still a real problem in SLY :-)

@joaotavora joaotavora reopened this Oct 14, 2018
@cireu
Copy link

cireu commented Sep 16, 2019

Another problem is, cl-indent allow you use different indentation for Elisp, by setting property common-lisp-indent-function-for-elisp of symbol common-lisp-indent-function. sly-cl-indent don't support this. It means if someone use common-lisp-indent-function to indent Elisp code(Following cl-indent's usage), their Elisp indentation may get broken after load sly.

@tarsius
Copy link
Contributor Author

tarsius commented Jan 19, 2020

There is a way out, but it's basically throwing in the towel and probably also hard: Rename every symbol in sly-cl-indent.el that conflicts with cl-indent.el, and stop providing cl-indent

At this point that's what I think you should do. (Except that you should rename each and every symbol.)

@SwiftLawnGnome
Copy link
Contributor

I've done this in my own fork, it wasn't as tedious as it seemed. Been using it for almost a month without issues. I didn't totally streamline the naming conventions (though all symbols do now start with sly) but that's easy to change. I'll happily submit a PR if you'd like, this has been my biggest pet peeve with Sly/Slime as long as I've used them.

@joaotavora
Copy link
Owner

I'll happily submit a PR if you'd like, this has been my biggest pet peeve with Sly/Slime as long as I've used them.

Sure @SwiftLawnGnome , I think that's a good idea, at least to have a starting point. Though a naming convention is useful.

@joaotavora
Copy link
Owner

@SwiftLawnGnome , I've had a look at your fork. Pretty large diff. You say it wasn't tedious?!! That's great! The naming convention you adopted doesn't seem terrible at all. Maybe, if you can manage, I would replace all the sly- with sly-- so we don't conceptually export anything outside of SLY, for now at least. But other than that, it looks pretty good. I would definitely like to have a pull request for this, so I can test it for a while.

Thanks very much

@SwiftLawnGnome
Copy link
Contributor

SwiftLawnGnome commented Apr 16, 2020

I've had a look at your fork. Pretty large diff. You say it wasn't tedious?!!

Well, it was tedious, but iedit and counsel-rg kept me sane!

Maybe, if you can manage, I would replace all the sly- with sly-- so we don't conceptually export anything outside of SLY

I just pushed a bunch of commits which changed most symbols. The sly--lisp-indent- prefix is used almost everywhere. Some external symbols may have the - that shouldn't and vice versa, let me know of any other naming changes you think are appropriate. I haven't tried these recent changes yet, and considering how many there are, there's a very good chance something's broken. I probably won't be able to do much testing atm but I'll go ahead and submit the PR.

Here: #312

@joaotavora
Copy link
Owner

I finally did the reviewing work and pushed this. @tarsius we have @SwiftLawnGnome to thank for this.

@SwiftLawnGnome , thanks very much: you did a lot of hard work, renaming and making everything consistent in the sly-indent-cl.el library and its users. It took me some time to review the most important commits, but I selected those and pushed them to master. I went through basically every little decision you took and agreed with all of them, no exception. You did introduce a minor bug with the "indent-13" test case that took me a while to figure out, but in the end I found it fixed it.

In #312 you have a bunch of unrelated, but also very interesting commits that I will push to master incrementally, as I review them. I will close that Pull Request, but don't remove that branch, as I will be cherry-picking the commits.

@joaotavora
Copy link
Owner

And it only took 4 years! :-D

@tarsius
Copy link
Contributor Author

tarsius commented Jun 29, 2020

Thank you @SwiftLawnGnome. 😉

And thanks to you too @joaotavora.

dsedivec added a commit to dsedivec/dot-emacs-d that referenced this issue Sep 10, 2020
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

4 participants