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

Clean ivy--regex-plus' to prepare to ivy--regex-plus-ignore-order' #344

Closed
wants to merge 2 commits into from

Conversation

@Konubinix
Copy link
Contributor

@Konubinix Konubinix commented Jan 12, 2016

The current implementation of ivy--regex-ignore-order' does not take into account the '!' character likeivy--regex-plus' does.

When trying to implement the '!' behavior in ivy--regex-ignore-order', I realized that it behaves more likeivy--regex' in the sense that it provides a way to create regex out of a string.

I also realized that ivy--regex-plus could be considered as a thin wrapper around "something that returns regex" and added the behavior of '!'. I thought that it could be a good idea to use it around ivy--regex-ignore-order also.

Unfortunately, ivy--regex-plus is difficult to work on since it returns either a string or a list of cons (let's call it a ivy style regex :-)). Also, it assumed that the called function (ivy--regex) returned a string and not an ivy style regex.

The purpose of this patch is to make `ivy--regex-plus' cleaner, not relying on the assumption that the called function returns a string.

To do that, I added ivy--regex-normalize that is fed with either a string or a ivy style regex and return a ivy style regex. Then ivy--regex-plus makes use of it.

I don't know yet how to customize ivy--regex-plus to use either ivy--regex or ivy--regex-ignore-order, but now, I feel that the base is clean enough to do this.

Konubinix added 2 commits Jan 12, 2016
It now only returns the part of the regex list that indicates to
ignore foo. The part accepting nothing is useless.
…cleaner

Now, `ivy--regex-plus' doesn't have anymore to construct the list and cons. It
relies on the `ivy--regex-normalize' and barely aggregate the result.
@abo-abo abo-abo force-pushed the abo-abo:master branch to 66e00ed Jan 12, 2016
@abo-abo abo-abo closed this in 89ed19e Jan 12, 2016
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jan 12, 2016

Thanks for the effort, but it's really not a good change:

  1. $ make test fails: swiper--re-builder depends on a certain structure of ivy--regex-plus.
  2. The ("" . t) part is necessary since "" is what will be used for swiper--add-overlays and in other places.
  3. The name ivy--regex-normalize implies that it maps a regex string to a regex string, which isn't so.
  4. In ivy--regex-plus, ivy--regex must not be called for the non-matching part, or at least it should be called before the call on the matching part. This is because ivy--regex isn't a pure function and has a side-effect of setting ivy--subexps.
  5. Finally, I like to introduce a new function when it's called at least twice (preferably more). The exception is breaking a function that doesn't fit on the screen into parts.

Please don't take it as offense, I usually try to clean up and salvage contributions, but there's nothing that I can keep from this one.

@Konubinix
Copy link
Contributor Author

@Konubinix Konubinix commented Jan 12, 2016

No problem. As mentioned, my initial attempt was to add the '!' behavior into ivy--regex-ignore-order. I though that this cleaning would help doing that, but I obviously did not understand all the impacts of the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants