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

Remove strict initialism code by @noctuid #94

Merged
merged 1 commit into from Jan 11, 2022

Conversation

minad
Copy link
Collaborator

@minad minad commented Nov 1, 2021

Omar, what do you think about the following finger excercise to fix the licensing situation in #57 to allow you to add Orderless to ELPA?

  1. We remove the strict initialism code.
  2. Someone with FSF assignment implements the removed functions and promises to not look at the solution. You should probably not do this since you already touched and refactored the relevant code.

I know it's crazy... Alternative is to just wait or add the package to NonGNU ELPA.

@iyefrat
Copy link

iyefrat commented Nov 1, 2021

Someone with FSF assignment implements the removed functions and promises to not look at the solution. You should probably not do this since you already touched and refactored the relevant code.

I can do that if you want. A bit unclear on what the code should do though.

@minad
Copy link
Collaborator Author

minad commented Nov 2, 2021

I can do that if you want. A bit unclear on what the code should do though.

It should build appropriate regexps which only match "strict" initialisms. Strict in the sense of anchoring the match to the boundary. Furthermore skipping words is forbidden. There are multiple types since one can either anchor to the left or the right or to both sides. It is also documented in the readme. As far as I understand it is important that one does not copy the code. A new implementation according to the specification remedies the licensing situation.

If you want to proceed, go for it. But of course it is up to Omar on what he wants to do replace code with equivalent code :-P For completeness, there are more alternatives: One could also just remove the strict initialism code, move it to the wiki. While I initially thought that I would be fine without strict initialism support, I now think that the feature is actually nice and we could even consider adding it to the default matching styles, since it does not have the performance issues of the initialism/flex style thanks to the anchoring! Furthermore initialism is mostly useful to type shortcuts in M-x, e.g., pifb for package-install-from-buffer or reb for report-emacs-bug. As a user one can learn these shortcuts so it is not much worse to use a strict initialism anchored to the beginning vs a non-strict initialism.

@oantolin
Copy link
Owner

oantolin commented Nov 2, 2021

I'm not sure about this whole "replacing code" approach yet. But if we do do it, I think we probably don't need more than one kind of strict initialism: how about keep just the kind anchored at the beginning? (We don't want it anchored at both ends since then it can't match until you finished typing the whole initialism!)

@minad
Copy link
Collaborator Author

minad commented Nov 2, 2021

Yes, I also wondered about the anchoring at both sides and at the end. These are not nice for interactive use. We could move those to the wiki. However they may still be useful to narrow down to an exact command. I don't bother much with these strict initialisms as I said, however it is nice that the strict initialism anchored at the beginning doesnt't suffer from the initialism performance issue.

The replacing code approach is definitely safe - it is a clean room implementation as long as the implementer does not look at the old code and old implements the specification.

@oantolin
Copy link
Owner

oantolin commented Nov 2, 2021

My worry about this clean room strategy isn't that it doesn't pass legal muster, I think I've been convinced it is allowed. I just don't want to disrespect contributors' wishes, and even if it is allowed it may seem rude or overly aggressive to the person whose code is replaced.

@minad
Copy link
Collaborator Author

minad commented Nov 2, 2021

Hmm, thinking about it again, we could also only keep the one which is not anchored at all. It will still not have the performance issues. But it will be less distinctive of course. Well for now I pushed a commit which removes everything and keeps only the one template. Maybe @iyefrat is interested in contributing this.

However I would like to make it clear that I don't want to step on @noctuid's feet here. Therefore my proposal would be to keep the existing functionality. My goal is just to resolve the licensing situation in the near future. If more than a single commit would be affected, ELPA wouldn't be feasible. But in this case we are talking about a single commit which is well-isolated.

@minad
Copy link
Collaborator Author

minad commented Nov 2, 2021

I just don't want to disrespect contributors' wishes, and even if it is allowed it may seem rude or overly aggressive to the person whose code is replaced.

Of course, I also worry about this and I am sorry for it. I cannot say much more than that. I still appreciate the ideas by @noctuid. But to be honest, @noctuid's state of the assignment in #57 didn't sound encouraging.

Note that from time to time one also rejects or removes features. I did that often in Consult for example, rejecting contributions or ideas and encouraging people to add the code in question to the wiki instead. I think it helps to keep some kind of objective/professional relationship towards your own code, such that you are not hurt if your code is not taken. The same applies to the code owner, who shouldn't be to protective towards their own code. So maybe I am guilty of that with regards to Consult. On the other hand many people expected Consult to precisely replicate Counsel, which was an explicit non-goal.

@noctuid
Copy link
Contributor

noctuid commented Nov 22, 2021

I am still working on getting assignment (just sent another email since I still have not received a response from legal team after providing the information they requested). I don't know if they need more information from me or the information I provided was sufficient to get a signature.

This is a bit silly/funny to me, but I understand. My contribution has already held up the process of getting orderless on ELPA for quite a while, and if that's going to continue to be the case, then I'm completely fine with my code being removed. I'd prefer if this branch was forked and the functionality re-implemented before merging into master. If not, I can add it back once I (hopefully) get assignment, but I think it would be better not to break backwards compatibility. I think this is better as part of orderless directly.

@minad
Copy link
Collaborator Author

minad commented Nov 22, 2021

@noctuid Thank you for responding here! What about keeping only the non-anchored variant? It seems to me that having the three variants (non-anchored, anchored at the beginning, anchored at both sides) is a bit of an overkill. Do you use all three variants? As I argued above, I see the usefulness of the non-anchored variant. But the other variants could as well live in personal user configurations or the orderless wiki. It also does not hurt to keep them here, but while we are on it it makes sense to at least discuss and reconsider this. Maybe the feature is only used by 1% of the users.

@oantolin
Copy link
Owner

Thanks for the update, @noctuid! Let's hope this gets resolved soon. In the meantime I wanted to float an idea:

A nice compromise between keeping only the non-anchored variant and keeping all variants, might be to keep a single strict initialism variant that you can anchor at one end or the other with ^ and $. How does that sound?

@minad
Copy link
Collaborator Author

minad commented Nov 22, 2021

@oantolin However one could argue that then it makes more sense to keep all variants and resolve this anchoring in the style dispatcher. I am not sure if I like losing configurability and hard coding it instead inside this single strict initialism matcher.

@noctuid
Copy link
Contributor

noctuid commented Nov 23, 2021

I don't care about the "full" initialism variant (I don't have a use case for it), but I would prefer to keep orderless-strict-leading-initialism. I think it's just as useful: I use it instead of non-anchored initialism for ~25 specific commands, e.g. anything like describe-variable. It's not much extra code, and I think is useful for enough builtin commands to justify keeping it.

@minad
Copy link
Collaborator Author

minad commented Jan 11, 2022

@oantolin How would you like or do you even want to proceed with this? I'd find it nice to have this on ELPA, preferably GNU ELPA. (Preferrable for these reasons: Maybe parts of GNU ELPA will be even bundled with Emacs for the distributions. Maybe more core packages will be moved to ELPA outside of the tree, and will only be added back for the distribution. Furthermore GNU ELPA can avoid duplication of work in case some core developer decides that something orderless-like should be in core.)

@oantolin
Copy link
Owner

To keep this from dragging on, let's remove the code and leave it on the wiki. We can always reimplement those matching styles later. How does that sound?

Furthermore GNU ELPA can avoid duplication of work in case some core developer decides that something orderless-like should be in core.

I'm fairly skeptical about this reason judging by several pairs of similar packages in core. 😛

The function `orderless--separated-by` has also been added by @noctuid. However
with the removal of the strict initialism code, the contributions by @noctuid
are pushed below the 15 lines limit. The function `orderless--separated-by` has
only 11 lines.

(cl-defun orderless--separated-by (sep rxs &optional (before "") (after ""))
  "Return a regexp to match the rx-regexps RXS with SEP in between.
If BEFORE is specified, add it to the beginning of the rx sequence. If AFTER is
specified, add it to the end of the rx sequence."
  (rx-to-string
   `(seq
     ,before
     ,@(cl-loop for (sexp . more) on rxs
                collect `(group ,sexp)
                when more collect `,sep)
     ,after)))
@minad
Copy link
Collaborator Author

minad commented Jan 11, 2022

I'm fairly skeptical about this reason judging by several pairs of similar packages in core.

You are right about this and I am of course also skeptical. However at least one has a good argument then. I would also say that many duplicated packages are historical.

I force pushed commit a which removes the strict initialism code.

@minad
Copy link
Collaborator Author

minad commented Jan 11, 2022

The question is if you want to add deprecation/removal warnings if these symbols are used?

I would maybe risk it, since the error will already be obvious. Furthermore I doubt that many people use these functions. We will see this in the following days. And maybe someone even steps up and proposes another PR adding them back.

@oantolin oantolin merged commit f22789f into oantolin:master Jan 11, 2022
@deejayem
Copy link

I would maybe risk it, since the error will already be obvious. Furthermore I doubt that many people use these functions. We will see this in the following days. And maybe someone even steps up and proposes another PR adding them back.

Since you're apparently expecting people to shout if they were using it... I was using orderless-strict-leading-initialism (and the error was obvious). I prefer it to orderless-initialism, but don't rely on it all that often anyway.

@minad
Copy link
Collaborator Author

minad commented Jan 12, 2022

@deejayem Sorry about the (hopefully temporary) hickup! You can copy the removed code to your configuration. It is really not that much code. There is the hope to put some of the code back, maybe only the strict leading initialism variant, which you use. It seems a bit excessive to have multiple variants.

@oantolin The Orderless ELPA debut succeeded: http://elpa.gnu.org/packages/orderless.html 🎉

@protesilaos
Copy link
Contributor

Just to echo @deejayem on orderless-strict-leading-initialism.

oantolin: We can always reimplement those matching styles later. How
does that sound?

[...]

minad: You can copy the removed code to your configuration. It is
really not that much code. There is the hope to put some of the code
back, maybe only the strict leading initialism variant, which you
use. It seems a bit excessive to have multiple variants.

I don't mind having the code in my config, though I wonder what the plan
is. If they will be reimplemented, then I can wait for them.

@minad
Copy link
Collaborator Author

minad commented Jan 15, 2022

I don't mind having the code in my config, though I wonder what the plan
is. If they will be reimplemented, then I can wait for them.

From how I've read the discussion, the most important matching style is orderless-strict-leading-initialism, which should be added back. The other variants are less important, so we also use the measure here to remove them, simplifying Orderless a little bit. Do you need all of the variants? In any case, special matching styles could be collected in the wiki.

Due to the ELPA copyright requirements we need a clean slate reimplementation, which could be done by everyone who didn't look at the old implementation of orderless-strict-leading-initialism before. However one is of course allowed to look at other similar functions like orderless-initialism. I'd say that Omar should probably not implement this himself since he already touched the old code. I could implement it I guess, but on the other hand it would also be a bit odd, since I just prepared the PR to remove the old implementation. @iyefrat offered before to help out, not sure if this offer still stands?

The informal spec according to which the function should be implemented can be taken out of the README:

- orderless-strict-initialism :: like initialism but only allow
  non-letters in between the matched words.

For example fb would match foo-bar but not foo-qux-bar.

- orderless-strict-leading-initialism :: like strict-initialism but
  require the first initial to match the candidate's first word.

For example bb would match bar-baz but not foo-bar-baz.

Sorry again for the inconvenience!

protesilaos added a commit to protesilaos/dotfiles that referenced this pull request Jan 15, 2022
@protesilaos
Copy link
Contributor

protesilaos commented Jan 15, 2022 via email

@iyefrat
Copy link

iyefrat commented Jan 18, 2022

Hey,
My FSF assignment status is currently in limbo due to personal circumstances, so I'm unable to do the clean room implementation at the moment. I'll update you if/when that changes.

@minad minad deleted the strict-initialism branch March 10, 2024 19:44
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

6 participants