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

Add s-replace-regexp #116

Merged
merged 1 commit into from
Nov 2, 2017
Merged

Add s-replace-regexp #116

merged 1 commit into from
Nov 2, 2017

Conversation

Silex
Copy link
Contributor

@Silex Silex commented Nov 1, 2017

Don't merge this yet.

@magnars: I need your guidance for the next points

Interface is quite different:

(s-replace OLD NEW S)
(s-replace-regexp REGEXP REP STRING &optional FIXEDCASE LITERAL SUBEXP START)

Do we want all the additional parameters?

Replace all matches for REGEXP with REP in STRING.

Return a new string containing the replacements.

Optional arguments FIXEDCASE, LITERAL and SUBEXP are like the
arguments with the same names of function ‘replace-match’.  If START
is non-nil, start replacements at that index in STRING.

REP is either a string used as the NEWTEXT arg of ‘replace-match’ or a
function.  If it is a function, it is called with the actual text of each
match, and its value is used as the replacement text.  When REP is called,
the match data are the result of matching REGEXP against a substring
of STRING, the same substring that is the actual text of the match which
is passed to REP as its argument.

To replace only the first match (if any), make REGEXP match up to \'
and replace a sub-expression, e.g.
  (replace-regexp-in-string "\\(foo\\).*\\'" "bar" " foo foo" nil nil 1)
    => " bar foo"

TODO:

  • Need to add more examples.
  • Need to decide wether we want to limit the additional parameters to s-replace-regexp.
  • Need to decide wether to implement s-replace-all-regexp.

@Silex Silex mentioned this pull request Nov 1, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-4.06%) to 95.313% when pulling 126b962 on Silex:master into 71f2902 on magnars:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.375% when pulling b6cdd53 on Silex:master into 71f2902 on magnars:master.

@magnars
Copy link
Owner

magnars commented Nov 1, 2017

I think it makes sense to keep the optional parameters. However, the order of the required parameters should match s-replace. In particular, note that all s- functions take the original string as the first argument, for ease of threading with s-with.

@Silex
Copy link
Contributor Author

Silex commented Nov 2, 2017

In particular, note that all s- functions take the original string as the first argument, for ease of threading with s-with.

Hum, sorry but https://github.com/magnars/s.el#s-replace-old-new-s says otherwise (oldtext, newtext, originalstring)?

I think it makes sense to keep the optional parameters. However, the order of the required parameters should match s-replace

I think it does as shown in my 1st post.

Anyway, that means we are good with (defalias 's-replace-regexp 'replace-regexp-in-string).

I'll add more example, in the meantime what do you think of s-replace-all-regexp? the way s-replace-all works is by building a giant "or" regexp and do one call to replace-regexp-in-string... I believe attempting the same could be problematic, but looping over the cons of replacements and calling N times replace-regexp-in-string could work. That or alternatively we don't implement s-replace-all-regexp at the moment.

@magnars
Copy link
Owner

magnars commented Nov 2, 2017

Yes, you are right. The original string is supposed to be last. All good then. I think s-replace-all-regexp needs a bit of thought, and can be skipped at the moment.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.375% when pulling 72ab5c0 on Silex:master into 71f2902 on magnars:master.

@Silex
Copy link
Contributor Author

Silex commented Nov 2, 2017

Alright, review the commit and suggest changes or merge 😉

@salamynder
Copy link

salamynder commented Nov 2, 2017

Thank you guys for putting so much work into this.

Regarding the necessity of s-replace-all-regexp:

The normal behaviour of replace-regexp-in-string replaces every occurence:

(replace-regexp-in-string "foo" "bar" "foo foo")
"bar bar"

(meaning: we don't need s-replace-regexp-all, but..); if this behaviour is not wished for one shall do subexpression replacement (last argument):

(replace-regexp-in-string "\\(foo\\).*\\'" "bar" " foo foo" nil nil 1)
    => " bar foo"

But this usage would again conflict with (s-with)-threading, because last arg isn't any longer the input-string but as said the subexpression number. (?!)

Overall problem: Cannot use &optional args of (replace-regexp-in-string) with (s-with)

@magnars
Copy link
Owner

magnars commented Nov 2, 2017

Overall problem: Cannot use &optional args of (replace-regexp-in-string) with (s-with)

Yes, this is the case for all functions with optional args. Unfortunately my early standardisation on having the original string in the last position bites us here.

@magnars
Copy link
Owner

magnars commented Nov 2, 2017

Looks good to me!

@magnars magnars merged commit 5e9a685 into magnars:master Nov 2, 2017
@Silex
Copy link
Contributor Author

Silex commented Nov 2, 2017

(meaning: we don't need s-replace-regexp-all, but..); if this behaviour is not wished for one shall do subexpression replacement (last argument):

I think you missunderstand s-replace-all-regexp, see https://github.com/magnars/s.el#s-replace-all-replacements-s

That is, s-replace already replaces all occurences.

@salamynder
Copy link

replacing all pairs... of course! sorry for the noise. And thanks for this fix!

@michaelmhoffman
Copy link

This is what I use as a replacement for s-replace-all-regexp:

(require 'dash)

(defun replace-regexp-cons (s replacement)
  "REPLACEMENT is a cons-cell. Each `car` is replaced with `cdr` in S.
n
`car` is a regexp.
`cdr` can include backreferences."
  (replace-regexp-in-string (car replacement) (cdr replacement) s t))

(defun replace-all-regexp (replacements s)
  "REPLACEMENTS is a list of cons-cells. Each `car` is replaced with `cdr` in S.

`car`s are regexps.
`cdr`s can include backreferences."
  (-reduce-from #'replace-regexp-cons s replacements))

It works well but has a dependency on dash.

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.

5 participants