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 hyperbole set functions - patch piece 3 #110

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

Conversation

matsl
Copy link
Collaborator

@matsl matsl commented Aug 4, 2021

What

The patch refactors use of the set functions in favor of using cl-lib functions that provides the same functionality. Some set functions remains though. So I'm not sure if this is a first step to remove set.el completely or if the remaining function is set.el still make sense to keep. Not sure about the role of set:equal-op in this scenario too since that one still remains.

A few other small unrelated but hopefully useful changes are kept in too to make the PR match Stefans patch better.

@matsl matsl requested a review from rswgnu August 4, 2021 09:38
@matsl matsl changed the title Refactor hyperbole set functions Refactor hyperbole set functions - patch part three Aug 4, 2021
@matsl matsl changed the title Refactor hyperbole set functions - patch part three Refactor hyperbole set functions - patch piece 3 Aug 4, 2021
@rswgnu
Copy link
Owner

rswgnu commented Oct 31, 2023

Mats, please put a comment here explaining the status on this one. Are we leaving it here for archival purposes? Does it have any value now with intent to pursue it in the future, etc.?

@matsl
Copy link
Collaborator Author

matsl commented Oct 31, 2023

Mats, please put a comment here explaining the status on this one. Are we leaving it here for archival purposes? Does it have any value now with intent to pursue it in the future, etc.?

I think we kept it as something we potentially would want to do but did not want to pursue at the time. Most important because we thought there were more important things to fix. Maybe time has come now to either drop it or act on it. I think there are multiple issues here (In my priority order):

  1. set is a pretty generic name so there is a high risk of collision with other packages. If I would see set in some code I would expect it to be part of core Emacs. This could be resolved easily with renaming file and functions to something like hypb-set (or whatever prefix we should use for Hyperbole.) (This would make externals happy.)
  2. The set functionality duplicates functionality provided by the cl-lib package so could either be replaced by it (as the current patch is doing) or could be used as a new implementation to simplify things.
  3. The way identity is used in the package is a nifty feature but do we need that? To me that complicates the package and a simple equals only implementation would be all we need and by that would be preferable IMHO. (I have not looked again now but I think we only use the equal version. Happy to be proven wrong here.)

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.

None yet

2 participants