[PATCH] lisp/files.el: Add `file-name-set-extension`

[PATCH] lisp/files.el: Add `file-name-set-extension`

[Colin Woodbury]

[-- Attachment #1.1: Type: text/plain, Size: 426 bytes --]

This patch adds a safe way to set a filename's extension. It sanitizes the input so that both these cases do the expected thing:

ELISP> (file-name-set-extension "jack.scss" "css")
"jack.css"

ELISP> (file-name-set-extension "jack.scss" ".css")
"jack.css"

Note that if we're trying to be safe with errors, nils, and empty strings, it's not sufficient to just `(concat (file-name-sans-extension file) "." extension)`.

Cheers!

[-- Attachment #1.2: Type: text/html, Size: 723 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: file-name-set-extension.patch --]
[-- Type: text/x-patch; name="file-name-set-extension.patch", Size: 1050 bytes --]

diff --git a/lisp/files.el b/lisp/files.el
index 62e1702fdf..f8aefa7930 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4889,6 +4889,20 @@ extension, the value is \"\"."
         (if period
             "")))))
 
+(defun file-name-set-extension (filename extension)
+  "Change the extension of a FILENAME to EXTENSION.
+Sanitizes the input to consolidate leading/trailing dots.
+
+Returns `nil' if either of the FILENAME or EXTENSION are `nil'
+before sanitizing, or empty afterwards."
+  (when (and filename extension)
+    (let* ((patt "[ \\t\\n\\r.]+") ; Borrowed from `string-trim'.
+           (filename (string-trim-right filename patt))
+           (extension (string-trim-left extension patt)))
+      (unless (or (string-empty-p filename)
+                  (string-empty-p extension))
+        (concat (file-name-sans-extension filename) "." extension)))))
+
 (defun file-name-base (&optional filename)
   "Return the base name of the FILENAME: no directory, no extension."
   (declare (advertised-calling-convention (filename) "27.1"))

Re: [PATCH] lisp/files.el: Add `file-name-set-extension`

[Andreas Schwab]

On Mai 25 2021, Colin Woodbury wrote:

> diff --git a/lisp/files.el b/lisp/files.el
> index 62e1702fdf..f8aefa7930 100644
> --- a/lisp/files.el
> +++ b/lisp/files.el
> @@ -4889,6 +4889,20 @@ extension, the value is \"\"."
>          (if period
>              "")))))
>  
> +(defun file-name-set-extension (filename extension)
> +  "Change the extension of a FILENAME to EXTENSION.
> +Sanitizes the input to consolidate leading/trailing dots.
> +
> +Returns `nil' if either of the FILENAME or EXTENSION are `nil'
> +before sanitizing, or empty afterwards."
> +  (when (and filename extension)

What is the use-case for nil?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] lisp/files.el: Add `file-name-set-extension`

[Colin Woodbury]

[-- Attachment #1: Type: text/plain, Size: 1144 bytes --]

Hi Andreas,

`nil` seemed more appropriate than returning either an empty string or throwing an error. The intent is to signal a failure in general, as it looks like other related functions in that file do.

On Tue, 25 May 2021, at 10:48, Andreas Schwab wrote:
> On Mai 25 2021, Colin Woodbury wrote:
> 
> > diff --git a/lisp/files.el b/lisp/files.el
> > index 62e1702fdf..f8aefa7930 100644
> > --- a/lisp/files.el
> > +++ b/lisp/files.el
> > @@ -4889,6 +4889,20 @@ extension, the value is \"\"."
> >          (if period
> >              "")))))
> >  
> > +(defun file-name-set-extension (filename extension)
> > +  "Change the extension of a FILENAME to EXTENSION.
> > +Sanitizes the input to consolidate leading/trailing dots.
> > +
> > +Returns `nil' if either of the FILENAME or EXTENSION are `nil'
> > +before sanitizing, or empty afterwards."
> > +  (when (and filename extension)
> 
> What is the use-case for nil?
> 
> Andreas.
> 
> -- 
> Andreas Schwab, schwab@linux-m68k.org <mailto:schwab%40linux-m68k.org>
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."
> 

[-- Attachment #2: Type: text/html, Size: 1945 bytes --]

Re: [PATCH] lisp/files.el: Add `file-name-set-extension`

[Basil L. Contovounesios]

"Colin Woodbury" <colin@fosskers.ca> writes:

> +    (let* ((patt "[ \\t\\n\\r.]+") ; Borrowed from `string-trim'.

Those backslashes need escaping only in string-trim's docstring, not in
the literal regexp string.

Thanks,

-- 
Basil

Re: [PATCH] lisp/files.el: Add `file-name-set-extension`

[Colin Woodbury]

[-- Attachment #1.1: Type: text/plain, Size: 492 bytes --]

Thank you, you're right. I've tested the old and revised code, and the double backslash wasn't neccesary.

I've attached the revised patch.

On Tue, 25 May 2021, at 11:00, Basil L. Contovounesios wrote:
> "Colin Woodbury" <colin@fosskers.ca <mailto:colin%40fosskers.ca>> writes:
> 
> > +    (let* ((patt "[ \\t\\n\\r.]+") ; Borrowed from `string-trim'.
> 
> Those backslashes need escaping only in string-trim's docstring, not in
> the literal regexp string.
> 
> Thanks,
> 
> -- 
> Basil
> 

[-- Attachment #1.2: Type: text/html, Size: 954 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: file-name-set-extension.patch --]
[-- Type: text/x-patch; name="file-name-set-extension.patch", Size: 1045 bytes --]

diff --git a/lisp/files.el b/lisp/files.el
index 62e1702fdf..18a54b3ec7 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4889,6 +4889,20 @@ extension, the value is \"\"."
         (if period
             "")))))
 
+(defun file-name-set-extension (filename extension)
+  "Change the extension of a FILENAME to EXTENSION.
+Sanitizes the input to consolidate leading/trailing dots.
+
+Returns `nil' if either of the FILENAME or EXTENSION are `nil'
+before sanitizing, or empty afterwards."
+  (when (and filename extension)
+    (let* ((patt "[ \t\n\r.]+") ; Inspired by `string-trim'.
+           (filename (string-trim-right filename patt))
+           (extension (string-trim-left extension patt)))
+      (unless (or (string-empty-p filename)
+                  (string-empty-p extension))
+        (concat (file-name-sans-extension filename) "." extension)))))
+
 (defun file-name-base (&optional filename)
   "Return the base name of the FILENAME: no directory, no extension."
   (declare (advertised-calling-convention (filename) "27.1"))

Re: [PATCH] lisp/files.el: Add `file-name-set-extension`

[Stefan Monnier]

> +  (when (and filename extension)
> +    (let* ((patt "[ \t\n\r.]+") ; Inspired by `string-trim'.
> +           (filename (string-trim-right filename patt))
> +           (extension (string-trim-left extension patt)))
> +      (unless (or (string-empty-p filename)
> +                  (string-empty-p extension))
> +        (concat (file-name-sans-extension filename) "." extension)))))

Why do you trim [ \t\n\r.]+ from the end of the filename and the
beginning of the extension?

I can see why you'd want to remove a single "." at the beginning of
`extension`, so as to allow extension to come with or without a leading
".", but the rest seems rather surprising because such characters in my
experience almost never show up in such a circumstance (which means
that if they do, it's either on purpose and we should preserve it, or
it's an error "upstream" and we should try and help expose the error
rather than silently try to cover it).


        Stefan

Re: [PATCH] lisp/files.el: Add `file-name-set-extension`

[Colin Woodbury]

[-- Attachment #1: Type: text/plain, Size: 2012 bytes --]

That regex was borrowed directly from `string-trim`, to which I added a `.`. You're right that the main motivation was to allow the caller to pass either `.foo` or `foo` as the extension and have either case work (many languages do this). Leaving the other characters in the regex seemed like a sanitary thing to do, just in case they pass something bogus. Although as the code is, there's nothing preventing them from still giving bogus characters before the file, or after the extension, making the resulting filepath still meaningless. 

So we could consider:

1. Simplifying the regex to just include dots (and perhaps whitespace) (i.e. trust the caller), or;
2. Expanding the logic to sanitize both ends of both arguments, (i.e. don't trust the caller), or;
3. Adding error-throwing logic if malformed files or extensions are given (i.e. warn the user).

I'm happy to alter the patch for any of those scenarios, whichever is preferable. Thanks!

On Tue, 25 May 2021, at 13:25, Stefan Monnier wrote:
> > +  (when (and filename extension)
> > +    (let* ((patt "[ \t\n\r.]+") ; Inspired by `string-trim'.
> > +           (filename (string-trim-right filename patt))
> > +           (extension (string-trim-left extension patt)))
> > +      (unless (or (string-empty-p filename)
> > +                  (string-empty-p extension))
> > +        (concat (file-name-sans-extension filename) "." extension)))))
> 
> Why do you trim [ \t\n\r.]+ from the end of the filename and the
> beginning of the extension?
> 
> I can see why you'd want to remove a single "." at the beginning of
> `extension`, so as to allow extension to come with or without a leading
> ".", but the rest seems rather surprising because such characters in my
> experience almost never show up in such a circumstance (which means
> that if they do, it's either on purpose and we should preserve it, or
> it's an error "upstream" and we should try and help expose the error
> rather than silently try to cover it).
> 
> 
>         Stefan
> 
> 

[-- Attachment #2: Type: text/html, Size: 2968 bytes --]

Re: [PATCH] lisp/files.el: Add `file-name-set-extension`

[Stefan Monnier]

> That regex was borrowed directly from `string-trim`, to which I added
> a `.`. You're right that the main motivation was to allow the caller to pass
> either `.foo` or `foo` as the extension and have either case work (many
> languages do this). Leaving the other characters in the regex seemed like
> a sanitary thing to do, just in case they pass something bogus.

File names can be used in different ways for different purposes.
We don't have any good reason to think that a space or a newline at the
end of a file name (or beginning of an extension) is "bogus".

If such things occurred somewhat often and always in ways where they are
indeed undesirable, we could consider removing them (the tradeoff
between occasional harm and frequent error-avoidance would be
favorable), but here this is just asking for trouble with no benefit.

> 1. Simplifying the regex to just include dots (and perhaps whitespace) (i.e. trust the caller), or;

Please do that.

> 2. Expanding the logic to sanitize both ends of both arguments, (i.e. don't trust the caller), or;

I don't think we have any reason to think we should "sanitize" it.

> 3. Adding error-throwing logic if malformed files or extensions are given (i.e. warn the user).

"Malformed" according to which standard?


        Stefan

Re: [PATCH] lisp/files.el: Add `file-name-set-extension`

[Colin Woodbury]

[-- Attachment #1.1: Type: text/plain, Size: 1534 bytes --]

Thanks Stefan, I've revised the patch according to your suggestions (see attached). I also revised the docstring.

Thanks!
Colin

On Tue, 25 May 2021, at 14:29, Stefan Monnier wrote:
> > That regex was borrowed directly from `string-trim`, to which I added
> > a `.`. You're right that the main motivation was to allow the caller to pass
> > either `.foo` or `foo` as the extension and have either case work (many
> > languages do this). Leaving the other characters in the regex seemed like
> > a sanitary thing to do, just in case they pass something bogus.
> 
> File names can be used in different ways for different purposes.
> We don't have any good reason to think that a space or a newline at the
> end of a file name (or beginning of an extension) is "bogus".
> 
> If such things occurred somewhat often and always in ways where they are
> indeed undesirable, we could consider removing them (the tradeoff
> between occasional harm and frequent error-avoidance would be
> favorable), but here this is just asking for trouble with no benefit.
> 
> > 1. Simplifying the regex to just include dots (and perhaps whitespace) (i.e. trust the caller), or;
> 
> Please do that.
> 
> > 2. Expanding the logic to sanitize both ends of both arguments, (i.e. don't trust the caller), or;
> 
> I don't think we have any reason to think we should "sanitize" it.
> 
> > 3. Adding error-throwing logic if malformed files or extensions are given (i.e. warn the user).
> 
> "Malformed" according to which standard?
> 
> 
>         Stefan
> 
> 

[-- Attachment #1.2: Type: text/html, Size: 2254 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: file-name-set-extension.patch --]
[-- Type: text/x-patch; name="file-name-set-extension.patch", Size: 1050 bytes --]

diff --git a/lisp/files.el b/lisp/files.el
index 62e1702fdf..ab8be6ad9e 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4889,6 +4889,21 @@ extension, the value is \"\"."
         (if period
             "")))))
 
+(defun file-name-set-extension (filename extension)
+  "Set the extension of a FILENAME to EXTENSION.
+Consolidates leading/trailing dots so that either `foo' or `.foo'
+can be passed as an EXTENSION.
+
+Returns nil if either of the FILENAME or EXTENSION are nil before
+dot consolidation, or empty afterwards."
+  (when (and filename extension)
+    (let* ((patt "[ .]+")
+           (filename (string-trim-right filename patt))
+           (extension (string-trim-left extension patt)))
+      (unless (or (string-empty-p filename)
+                  (string-empty-p extension))
+        (concat (file-name-sans-extension filename) "." extension)))))
+
 (defun file-name-base (&optional filename)
   "Return the base name of the FILENAME: no directory, no extension."
   (declare (advertised-calling-convention (filename) "27.1"))

Re: [PATCH] lisp/files.el: Add `file-name-set-extension`

[Andreas Schwab]

On Mai 25 2021, Colin Woodbury wrote:

> +Returns nil if either of the FILENAME or EXTENSION are nil before
> +dot consolidation, or empty afterwards."
> +  (when (and filename extension)

I still don't see any reason to allow nil.  A file name is always a
string.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] lisp/files.el: Add `file-name-set-extension`

[Colin Woodbury]

[-- Attachment #1: Type: text/plain, Size: 1479 bytes --]

Hi Andreas,

In this particular case, the `string-trim-*` family of functions panic with the rather unhelpful `Wrong type argument: stringp, nil` message when passed `nil`, so the initial `when` is guarding against that. Since `nil` exists as a "bottom" value to every type in Elisp (not unlike C's null pointers), we adopt the C approach of "don't trust what we're given, and expect the user to check what they're returned". Such scenarios are precisely what macros like `when-let` are for, as far as I can tell.

In general this approach is founded upon the ideas that:

1. `nil` is a good general signal for logical failure (for languages that lack generics, etc.).
2. Library code shouldn't crash/panic (but user code should feel free to).
3. It's okay to return `nil` if the docstrings are clear about when that happens.

Please let me know if I've overlooked something, I'm happy to revise the patch.

Cheers,
Colin

On Wed, 26 May 2021, at 00:34, Andreas Schwab wrote:
> On Mai 25 2021, Colin Woodbury wrote:
> 
> > +Returns nil if either of the FILENAME or EXTENSION are nil before
> > +dot consolidation, or empty afterwards."
> > +  (when (and filename extension)
> 
> I still don't see any reason to allow nil.  A file name is always a
> string.
> 
> Andreas.
> 
> -- 
> Andreas Schwab, schwab@linux-m68k.org <mailto:schwab%40linux-m68k.org>
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."
> 

[-- Attachment #2: Type: text/html, Size: 2146 bytes --]

Re: [PATCH] lisp/files.el: Add `file-name-set-extension`

[Andreas Schwab]

That's exactly what should happen.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] lisp/files.el: Add `file-name-set-extension`

[Colin Woodbury]

[-- Attachment #1.1: Type: text/plain, Size: 640 bytes --]

Hi Andreas (et al.),

After asking around, it seems like letting `string-trim` fail naturally is indeed the canonical thing to do here, especially since with `toggle-debug-on-error` we're able to get the full call-stack if we need it. I've altered the patch (see attached) accordingly. Thanks for your patience.

Cheers,
Colin

On Wed, 26 May 2021, at 10:39, Andreas Schwab wrote:
> That's exactly what should happen.
> 
> Andreas.
> 
> -- 
> Andreas Schwab, schwab@linux-m68k.org <mailto:schwab%40linux-m68k.org>
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."
> 

[-- Attachment #1.2: Type: text/html, Size: 1092 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: file-name-set-extension.patch --]
[-- Type: text/x-patch; name="file-name-set-extension.patch", Size: 789 bytes --]

diff --git a/lisp/files.el b/lisp/files.el
index 62e1702fdf..eed818e2fa 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4889,6 +4889,15 @@ extension, the value is \"\"."
         (if period
             "")))))
 
+(defun file-name-set-extension (filename extension)
+  "Set the EXTENSION of a FILENAME.
+Consolidates leading/trailing dots so that either `foo' or `.foo'
+can be passed as an EXTENSION."
+  (let* ((patt "[ .]+")
+         (filename (string-trim-right filename patt))
+         (extension (string-trim-left extension patt)))
+    (concat (file-name-sans-extension filename) "." extension)))
+
 (defun file-name-base (&optional filename)
   "Return the base name of the FILENAME: no directory, no extension."
   (declare (advertised-calling-convention (filename) "27.1"))

Re: [PATCH] lisp/files.el: Add `file-name-set-extension`

[Philipp]

> Am 31.05.2021 um 03:16 schrieb Colin Woodbury <colin@fosskers.ca>:
> 
> Hi Andreas (et al.),
> 
> After asking around, it seems like letting `string-trim` fail naturally is indeed the canonical thing to do here, especially since with `toggle-debug-on-error` we're able to get the full call-stack if we need it. I've altered the patch (see attached) accordingly. Thanks for your patience.
> 
> Cheers,
> Colin
> 
> On Wed, 26 May 2021, at 10:39, Andreas Schwab wrote:
>> That's exactly what should happen.
>> 
>> Andreas.
>> 
>> -- 
>> Andreas Schwab, schwab@linux-m68k.org
>> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
>> "And now for something completely different."
>> 
> 
> <file-name-set-extension.patch>


Some general comments:

- Please add unit tests, especially testing corner cases such as empty strings, directory names, filenames starting with a dot, etc.
- I'd rename the function to something that doesn't contain "set".  "set" gives the impression that the function modifies the filename in place, which it doesn't do.  Likewise, change the docstring to clarify that a new string is returned.

Re: [PATCH] lisp/files.el: Add `file-name-set-extension`

[Colin Woodbury]

[-- Attachment #1.1: Type: text/plain, Size: 1301 bytes --]

Hi all, thanks for the input. I've revised the patch to account for empty strings and for filenames that look like directories (i.e. that end in a slash).

I've also begun the copyright assignment process, since adding the unit tests lengthened the patch past the critical line count.

As for the naming of `set-extension`, the best I can think of as an alternative would be `with-extension` as a dual to `sans-extension`, although I think `set` still signals the intent well enough. I'll defer to others here.

Cheers,
Colin

On Fri, 4 Jun 2021, at 06:23, Mattias Engdegård wrote:
> 4 juni 2021 kl. 15.08 skrev Philipp <p.stephani2@gmail.com <mailto:p.stephani2%40gmail.com>>:
> 
> > Likewise, change the docstring to clarify that a new string is returned.
> 
> Actually we usually don't bother saying that a function creates a new string unless this is guaranteed and somehow important, since we don't encourage string mutation anyway. For example, functions are usually allowed to return an unchanged argument string if that would make sense.
> 
> Conversely, only functions that do mutate strings need to mention that fact. By default, a user should be able to assume that no string mutation takes place.
> 
> I fully agree with the rest of Philipp's excellent points.
> 
> 

[-- Attachment #1.2: Type: text/html, Size: 1778 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: file-name-set-extension.patch --]
[-- Type: text/x-patch; name="file-name-set-extension.patch", Size: 2590 bytes --]

diff --git a/lisp/files.el b/lisp/files.el
index 2450daf5bf..f37264d7d5 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4892,6 +4892,18 @@ extension, the value is \"\"."
         (if period
             "")))))
 
+(defun file-name-set-extension (filename extension)
+  "Set the EXTENSION of a FILENAME.
+Consolidates leading/trailing dots so that either `foo' or `.foo'
+can be passed as an EXTENSION."
+  (let* ((patt "[ .]+")
+         (file (string-trim-right filename patt))
+         (extn (string-trim-left extension patt)))
+    (cond ((string-empty-p file) (error "Malformed filename: %s" filename))
+          ((string-empty-p extn) (error "Malformed extension: %s" extension))
+          ((equal ?/ (string-to-char (substring file -1))) (error "Filename is a directory: %s" filename))
+          (t (concat (file-name-sans-extension file) "." extn)))))
+
 (defun file-name-base (&optional filename)
   "Return the base name of the FILENAME: no directory, no extension."
   (declare (advertised-calling-convention (filename) "27.1"))
diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index dc96dff639..92bbd95eaf 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -1478,5 +1478,25 @@ The door of all subtleties!
                                (buffer-substring (point-min) (point-max))
                                nil nil)))))
 
+(ert-deftest files-tests-file-name-set-extension-good ()
+  "Test that `file-name-set-extension' succeeds with reasonable input."
+  (should (string= (file-name-set-extension "Jack" "css") "Jack.css"))
+  (should (string= (file-name-set-extension "Jack" ".css") "Jack.css"))
+  (should (string= (file-name-set-extension "Jack.scss" "css") "Jack.css"))
+  (should (string= (file-name-set-extension "Jack..." "...css") "Jack.css"))
+  (should (string= (file-name-set-extension "/path/to/Jack.md" "org") "/path/to/Jack.org")))
+
+(ert-deftest files-tests-file-name-set-extension-bad ()
+  "Test that `file-name-set-extension' fails on malformed input."
+  (should-error (file-name-set-extension nil nil))
+  (should-error (file-name-set-extension "Jack" nil))
+  (should-error (file-name-set-extension nil "css"))
+  (should-error (file-name-set-extension "" ""))
+  (should-error (file-name-set-extension "" "css"))
+  (should-error (file-name-set-extension "Jack" ""))
+  (should-error (file-name-set-extension "Jack" "..."))
+  (should-error (file-name-set-extension "..." "css"))
+  (should-error (file-name-set-extension "/is/a/directory/" "css")))
+
 (provide 'files-tests)
 ;;; files-tests.el ends here

Re: [PATCH] lisp/files.el: Add `file-name-set-extension`

[Stefan Monnier]

> As for the naming of `set-extension`, the best I can think of as an
> alternative would be `with-extension` as a dual to `sans-extension`,

`file-name-with-extension` sounds good to me (better than `...set...`).


        Stefan

Re: [PATCH] lisp/files.el: Add `file-name-set-extension`

[Colin Woodbury]

[-- Attachment #1.1: Type: text/plain, Size: 345 bytes --]

So be it! I've updated the patch.

Colin

On Wed, 9 Jun 2021, at 16:05, Stefan Monnier wrote:
> > As for the naming of `set-extension`, the best I can think of as an
> > alternative would be `with-extension` as a dual to `sans-extension`,
> 
> `file-name-with-extension` sounds good to me (better than `...set...`).
> 
> 
>         Stefan
> 
> 

[-- Attachment #1.2: Type: text/html, Size: 769 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: file-name-with-extension.patch --]
[-- Type: text/x-patch; name="file-name-with-extension.patch", Size: 2649 bytes --]

diff --git a/lisp/files.el b/lisp/files.el
index 2450daf5bf..ad04386cc2 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4892,6 +4892,20 @@ extension, the value is \"\"."
         (if period
             "")))))
 
+(defun file-name-with-extension (filename extension)
+  "Set the EXTENSION of a FILENAME.
+Consolidates leading/trailing dots so that either `foo' or `.foo'
+can be passed as an EXTENSION.
+
+See also `file-name-sans-extension'."
+  (let* ((patt "[ .]+")
+         (file (string-trim-right filename patt))
+         (extn (string-trim-left extension patt)))
+    (cond ((string-empty-p file) (error "Malformed filename: %s" filename))
+          ((string-empty-p extn) (error "Malformed extension: %s" extension))
+          ((equal ?/ (string-to-char (substring file -1))) (error "Filename is a directory: %s" filename))
+          (t (concat (file-name-sans-extension file) "." extn)))))
+
 (defun file-name-base (&optional filename)
   "Return the base name of the FILENAME: no directory, no extension."
   (declare (advertised-calling-convention (filename) "27.1"))
diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index dc96dff639..80f47a78dc 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -1478,5 +1478,25 @@ The door of all subtleties!
                                (buffer-substring (point-min) (point-max))
                                nil nil)))))
 
+(ert-deftest files-tests-file-name-with-extension-good ()
+  "Test that `file-name-with-extension' succeeds with reasonable input."
+  (should (string= (file-name-with-extension "Jack" "css") "Jack.css"))
+  (should (string= (file-name-with-extension "Jack" ".css") "Jack.css"))
+  (should (string= (file-name-with-extension "Jack.scss" "css") "Jack.css"))
+  (should (string= (file-name-with-extension "Jack..." "...css") "Jack.css"))
+  (should (string= (file-name-with-extension "/path/to/Jack.md" "org") "/path/to/Jack.org")))
+
+(ert-deftest files-tests-file-name-with-extension-bad ()
+  "Test that `file-name-with-extension' fails on malformed input."
+  (should-error (file-name-with-extension nil nil))
+  (should-error (file-name-with-extension "Jack" nil))
+  (should-error (file-name-with-extension nil "css"))
+  (should-error (file-name-with-extension "" ""))
+  (should-error (file-name-with-extension "" "css"))
+  (should-error (file-name-with-extension "Jack" ""))
+  (should-error (file-name-with-extension "Jack" "..."))
+  (should-error (file-name-with-extension "..." "css"))
+  (should-error (file-name-with-extension "/is/a/directory/" "css")))
+
 (provide 'files-tests)
 ;;; files-tests.el ends here

Re: [PATCH] lisp/files.el: Add `file-name-set-extension`

[Colin Woodbury]

[-- Attachment #1.1: Type: text/plain, Size: 636 bytes --]

And while I'm at, I've tweaked one line to use `aref` to access the final character of the filename directly, which avoids a (albeit minor) string allocation. 

On Wed, 9 Jun 2021, at 17:42, Colin Woodbury wrote:
> So be it! I've updated the patch.
> 
> Colin
> 
> On Wed, 9 Jun 2021, at 16:05, Stefan Monnier wrote:
>> > As for the naming of `set-extension`, the best I can think of as an
>> > alternative would be `with-extension` as a dual to `sans-extension`,
>> 
>> `file-name-with-extension` sounds good to me (better than `...set...`).
>> 
>> 
>>         Stefan
>> 
>> 
> 
> 
> *Attachments:*
>  * file-name-with-extension.patch

[-- Attachment #1.2: Type: text/html, Size: 1199 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: file-name-with-extension.patch --]
[-- Type: text/x-patch; name="file-name-with-extension.patch", Size: 2643 bytes --]

diff --git a/lisp/files.el b/lisp/files.el
index 2450daf5bf..c6938eb1c8 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4892,6 +4892,20 @@ extension, the value is \"\"."
         (if period
             "")))))
 
+(defun file-name-with-extension (filename extension)
+  "Set the EXTENSION of a FILENAME.
+Consolidates leading/trailing dots so that either `foo' or `.foo'
+can be passed as an EXTENSION.
+
+See also `file-name-sans-extension'."
+  (let* ((patt "[ .]+")
+         (file (string-trim-right filename patt))
+         (extn (string-trim-left extension patt)))
+    (cond ((string-empty-p file) (error "Malformed filename: %s" filename))
+          ((string-empty-p extn) (error "Malformed extension: %s" extension))
+          ((equal ?/ (aref file (1- (length file)))) (error "Filename is a directory: %s" filename))
+          (t (concat (file-name-sans-extension file) "." extn)))))
+
 (defun file-name-base (&optional filename)
   "Return the base name of the FILENAME: no directory, no extension."
   (declare (advertised-calling-convention (filename) "27.1"))
diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index dc96dff639..80f47a78dc 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -1478,5 +1478,25 @@ The door of all subtleties!
                                (buffer-substring (point-min) (point-max))
                                nil nil)))))
 
+(ert-deftest files-tests-file-name-with-extension-good ()
+  "Test that `file-name-with-extension' succeeds with reasonable input."
+  (should (string= (file-name-with-extension "Jack" "css") "Jack.css"))
+  (should (string= (file-name-with-extension "Jack" ".css") "Jack.css"))
+  (should (string= (file-name-with-extension "Jack.scss" "css") "Jack.css"))
+  (should (string= (file-name-with-extension "Jack..." "...css") "Jack.css"))
+  (should (string= (file-name-with-extension "/path/to/Jack.md" "org") "/path/to/Jack.org")))
+
+(ert-deftest files-tests-file-name-with-extension-bad ()
+  "Test that `file-name-with-extension' fails on malformed input."
+  (should-error (file-name-with-extension nil nil))
+  (should-error (file-name-with-extension "Jack" nil))
+  (should-error (file-name-with-extension nil "css"))
+  (should-error (file-name-with-extension "" ""))
+  (should-error (file-name-with-extension "" "css"))
+  (should-error (file-name-with-extension "Jack" ""))
+  (should-error (file-name-with-extension "Jack" "..."))
+  (should-error (file-name-with-extension "..." "css"))
+  (should-error (file-name-with-extension "/is/a/directory/" "css")))
+
 (provide 'files-tests)
 ;;; files-tests.el ends here

Re: [PATCH] lisp/files.el: Add `file-name-set-extension`

[Michael Albinus]

"Colin Woodbury" <colin@fosskers.ca> writes:

> Hi all,

Hi Colin,

> +          ((equal ?/ (string-to-char (substring file -1))) (error "Filename is a directory: %s" filename))

This shall be (directory-name-p file) . You cannot assume that ?/ is
always the directory separator, for example on MS Windows.

Best regards, Michael.

Re: [PATCH] lisp/files.el: Add `file-name-set-extension`

[Colin Woodbury]

[-- Attachment #1.1: Type: text/plain, Size: 555 bytes --]

Good catch Michael, thank you, I had completely forgotten about Windows.  I've updated the patch.

Cheers,
Colin

On Thu, 10 Jun 2021, at 00:46, Michael Albinus wrote:
> "Colin Woodbury" <colin@fosskers.ca <mailto:colin%40fosskers.ca>> writes:
> 
> > Hi all,
> 
> Hi Colin,
> 
> > +          ((equal ?/ (string-to-char (substring file -1))) (error "Filename is a directory: %s" filename))
> 
> This shall be (directory-name-p file) . You cannot assume that ?/ is
> always the directory separator, for example on MS Windows.
> 
> Best regards, Michael.
> 

[-- Attachment #1.2: Type: text/html, Size: 1074 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: file-name-with-extension.patch --]
[-- Type: text/x-patch; name="file-name-with-extension.patch", Size: 2625 bytes --]

diff --git a/lisp/files.el b/lisp/files.el
index 2450daf5bf..5fb9b9dcdc 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4892,6 +4892,20 @@ extension, the value is \"\"."
         (if period
             "")))))
 
+(defun file-name-with-extension (filename extension)
+  "Set the EXTENSION of a FILENAME.
+Consolidates leading/trailing dots so that either `foo' or `.foo'
+can be passed as an EXTENSION.
+
+See also `file-name-sans-extension'."
+  (let* ((patt "[ .]+")
+         (file (string-trim-right filename patt))
+         (extn (string-trim-left extension patt)))
+    (cond ((string-empty-p file) (error "Malformed filename: %s" filename))
+          ((string-empty-p extn) (error "Malformed extension: %s" extension))
+          ((directory-name-p file) (error "Filename is a directory: %s" filename))
+          (t (concat (file-name-sans-extension file) "." extn)))))
+
 (defun file-name-base (&optional filename)
   "Return the base name of the FILENAME: no directory, no extension."
   (declare (advertised-calling-convention (filename) "27.1"))
diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index dc96dff639..80f47a78dc 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -1478,5 +1478,25 @@ The door of all subtleties!
                                (buffer-substring (point-min) (point-max))
                                nil nil)))))
 
+(ert-deftest files-tests-file-name-with-extension-good ()
+  "Test that `file-name-with-extension' succeeds with reasonable input."
+  (should (string= (file-name-with-extension "Jack" "css") "Jack.css"))
+  (should (string= (file-name-with-extension "Jack" ".css") "Jack.css"))
+  (should (string= (file-name-with-extension "Jack.scss" "css") "Jack.css"))
+  (should (string= (file-name-with-extension "Jack..." "...css") "Jack.css"))
+  (should (string= (file-name-with-extension "/path/to/Jack.md" "org") "/path/to/Jack.org")))
+
+(ert-deftest files-tests-file-name-with-extension-bad ()
+  "Test that `file-name-with-extension' fails on malformed input."
+  (should-error (file-name-with-extension nil nil))
+  (should-error (file-name-with-extension "Jack" nil))
+  (should-error (file-name-with-extension nil "css"))
+  (should-error (file-name-with-extension "" ""))
+  (should-error (file-name-with-extension "" "css"))
+  (should-error (file-name-with-extension "Jack" ""))
+  (should-error (file-name-with-extension "Jack" "..."))
+  (should-error (file-name-with-extension "..." "css"))
+  (should-error (file-name-with-extension "/is/a/directory/" "css")))
+
 (provide 'files-tests)
 ;;; files-tests.el ends here

Re: [PATCH] lisp/files.el: Add `file-name-set-extension`

[Colin Woodbury]

[-- Attachment #1: Type: text/plain, Size: 799 bytes --]

The copyright assignment process was finalized for me today, so this should be good-to-go.

Thanks!
Colin

On Thu, 10 Jun 2021, at 07:40, Colin Woodbury wrote:
> Good catch Michael, thank you, I had completely forgotten about Windows.  I've updated the patch.
> 
> Cheers,
> Colin
> 
> On Thu, 10 Jun 2021, at 00:46, Michael Albinus wrote:
>> "Colin Woodbury" <colin@fosskers.ca <mailto:colin%40fosskers.ca>> writes:
>> 
>> > Hi all,
>> 
>> Hi Colin,
>> 
>> > +          ((equal ?/ (string-to-char (substring file -1))) (error "Filename is a directory: %s" filename))
>> 
>> This shall be (directory-name-p file) . You cannot assume that ?/ is
>> always the directory separator, for example on MS Windows.
>> 
>> Best regards, Michael.
>> 
> 
> 
> *Attachments:*
>  * file-name-with-extension.patch

[-- Attachment #2: Type: text/html, Size: 1493 bytes --]

Re: [PATCH] lisp/files.el: Add `file-name-set-extension`

[Michael Albinus]

"Colin Woodbury" <colin@fosskers.ca> writes:

Hi Colin,

> The copyright assignment process was finalized for me today, so this
> should be good-to-go.

I've pushed your patch to master. I've added also the required
ChangeLog-like commit message, perhaps you could do this yourself with
the next patch?

What is missing is the documentation of the function in doc/lispref/files.texi
and an entry in etc/NEWS. Could you provide these as another patch?

> Thanks!
> Colin

Best regards, Michael.