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

Refactor aas-post-self-insert-hook #20

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

aikrahguzar
Copy link
Contributor

Hi!
I have had this changed version in my fork for a long while but somehow never tried to make a pull request out of it. As far as I can tell it is equivalent to the current version but simpler to understand at least for me and has worked correctly for me.

I saw the recent activity on the package and thought it might be a good opportunity to get rid of my own version. Please let me know what you think.

@ymarco
Copy link
Owner

ymarco commented Mar 8, 2023

Looks ok. In my original code I tried to not call cons and create garbage unless I had to, which is why I modified the prefix maps in-place.
Do you think we could do it with cl-loop instead? That would be peak readability. After writing this package I found out that cl-loop has in-ref to help modify lists in-place.

@aikrahguzar
Copy link
Contributor Author

Looks ok. In my original code I tried to not call cons and create garbage unless I had to, which is why I modified the prefix maps in-place. Do you think we could do it with cl-loop instead? That would be peak readability. After writing this package I found out that cl-loop has in-ref to help modify lists in-place.

I did it with cl-loop and it is even shorter and doesn't do the consing and works for some quick testing I have done.

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.

2 participants