Hi,
Xinglu Chen writes:
> * f.scm (replace-extension): New procedure.
Nice idea, and thanks for looking into this, 'cause I even forgot I had
this project!
I tried running this and it seems to behave perfectly when it's used on
a file that does indeed have an extension:
--8<---------------cut here---------------start------------->8---
(replace-extension "asdf.txt" "defg")
=> "asdf.defg"
--8<---------------cut here---------------end--------------->8---
But it seems to throw an ugly exception when FILE has no dot in it, or
when the dot is the last character:
--8<---------------cut here---------------start------------->8---
(replace-extension "asdf." "defg")
ice-9/boot-9.scm:1669:16: In procedure raise-exception:
In procedure vector-ref: Wrong type argument in position 1 (expecting vector): #f
(replace-extension "asdf" "defg")
ice-9/boot-9.scm:1669:16: In procedure raise-exception:
In procedure vector-ref: Wrong type argument in position 1 (expecting vector): #f
--8<---------------cut here---------------end--------------->8---
I looked into the equivalent function from f.el [1] and I think its
behaviour is a bit more user friendly.
--8<---------------cut here---------------start------------->8---
(f-swap-ext "asdf.txt" "defg")
=> "asdf.defg"
(f-swap-ext "asdf." "defg")
=> "asdf.defg"
(f-swap-ext "asdf" "defg")
=> "asdf.defg"
--8<---------------cut here---------------end--------------->8---
The only thing that I don't really find that intuitive about it is that
when the extension is the empty string, instead of returning the file
name without an extension (as I thought it would), it gives an error:
--8<---------------cut here---------------start------------->8---
(f-swap-ext "asdf.txt" "")
=> Lisp error: (error "Extension cannot be empty or nil")
--8<---------------cut here---------------end--------------->8---
Let me know what you think about this.
[1]: https://github.com/rejeep/f.el#f-swap-ext-path-ext
--
Alexandru-Sergiu Marton
https://brown.121407.xyz
On Tue, Mar 02 2021, Alexandru-Sergiu Marton wrote:
> I looked into the equivalent function from f.el [1] and I think its> behaviour is a bit more user friendly.>> --8<---------------cut here---------------start------------->8---> (f-swap-ext "asdf.txt" "defg")> => "asdf.defg">> (f-swap-ext "asdf." "defg")> => "asdf.defg">> (f-swap-ext "asdf" "defg")> => "asdf.defg"> --8<---------------cut here---------------end--------------->8---
I haven't looked at f.el yet, but this behaviour does indeed feel nicer
than giving a cryptic error message.
> The only thing that I don't really find that intuitive about it is that> when the extension is the empty string, instead of returning the file> name without an extension (as I thought it would), it gives an error:>> --8<---------------cut here---------------start------------->8---> (f-swap-ext "asdf.txt" "")> => Lisp error: (error "Extension cannot be empty or nil")> --8<---------------cut here---------------end--------------->8--->> Let me know what you think about this.
I agree that it should probably just return the filename if the
extension is emtpy. This behaviour would be more liberal and consistent
with the other cases IMO.
Thanks for the feedback!
Also, what naming convention should we use? f.el seems to prefer
shorter names like "f-no-ext" rather than "f-no-extension". I guess we
don't need the "f-" prefix since users can import the module with their
preferred prefix anyway.
Sorry for the terrible formatting, I'm on my phone.
2 Mar 2021 22:56:54 Xinglu Chen <public@yoctocell.xyz>:
> Also, what naming convention should we use? f.el seems to prefer> shorter names like "f-no-ext" rather than "f-no-extension". I guess we> don't need the "f-" prefix since users can import the module with their> preferred prefix anyway.
Yeah, we don't need the any kind of implicit prefix like "f-".
Most of the Scheme code I read was quite verbose. Take, for example, this
function name from Guix: "specifications->manifest" — it could have been
"specs->manifest" or something like that, but it seems Scheme people
prefer to be clear about stuff. So I think having full words in the name
is the way to go. It also flows better when reading the code, it sounds
like English without any extra in-brain processing needed.
--
Alexandru-Sergiu Marton
On Tue, Mar 02 2021, Alexandru-Sergiu Marton wrote:
> Sorry for the terrible formatting, I'm on my phone.
No worries, it actually looks fine to me. :)
> 2 Mar 2021 22:56:54 Xinglu Chen <public@yoctocell.xyz>:>>> Also, what naming convention should we use? f.el seems to prefer>> shorter names like "f-no-ext" rather than "f-no-extension". I guess we>> don't need the "f-" prefix since users can import the module with their>> preferred prefix anyway.>> Yeah, we don't need the any kind of implicit prefix like "f-".>> Most of the Scheme code I read was quite verbose. Take, for example,> this function name from Guix: "specifications->manifest" — it could> have been "specs->manifest" or something like that, but it seems> Scheme people prefer to be clear about stuff. So I think having full> words in the name is the way to go. It also flows better when reading> the code, it sounds like English without any extra in-brain processing> needed.
Yeah, most Scheme functions tend to have longer names, I will do the
same then.
Hello again,
Xinglu Chen writes:
> * f.scm (replace-extension): New procedure.> (file-extension): Likewise.> (file-no-extension): Likewise.> ---> Changes since v1:> - Make `replace-extension` more liberal instead of giving exceptions.> - Add `file-extension` and `file-no-extension` which are useful on> their own.>> f.scm | 29 ++++++++++++++++++++++++++++-> 1 file changed, 28 insertions(+), 1 deletion(-)>> diff --git a/f.scm b/f.scm> index f699d91..9e845d1 100644> --- a/f.scm> +++ b/f.scm> @@ -17,6 +17,7 @@> #:use-module ((ice-9 binary-ports) #:prefix i9:)> #:use-module ((ice-9 textual-ports) #:prefix i9:)> #:use-module (srfi srfi-1)> + #:use-module (ice-9 regex)> #:use-module ((f ports) #:prefix p:)> #:use-module ((f re-exports) #:prefix re:)> #:export (read-bytes> @@ -31,7 +32,10 @@> mkdir> delete> traverse> - copy)> + copy> + file-extension> + file-no-extension> + replace-extension)
You named the replacement function "replace-extension", but the others
start with "file-". All of them are related to file extensions. What do
you say about just removing the "file-" prefix on the first two? It
would still be clear what they do IMO, because the only thing that comes
to my mind when using a filesystem API and it has an "extension"
function is that it's related to file extensions.
> #:re-export (chown> chmod> (rename-file . move)> @@ -169,3 +173,26 @@ appending a #t at the end."))))))> (throw 'f.scm "Can't copy: directory not empty. Try with making the call recursive> by appending a #t at the end."))))> (copy-file src dest)))> +> +(define (file-extension file)> + "Return extension for FILE."> + (let ((ext (string-match ".*\\.(.*)" file)))> + (cond> + ((string-null? file) #f)> + (ext (regexp-substitute #f ext 1))> + (else file))))
(f:file-extension "file")
=> "file"
This seems to return FILE if there is no dot in it. I would expect it to
return "", because there is no extension in that case.
Also, it seems to think that hidden files have their whole name as an
extension, and that isn't right.
(f:file-extension ".hidden-file")
=> "hidden-file"
This should return "", as ".hidden-file" is a file with no extension. It
works correctly on hidden files *with* extensions:
(f:file-extension ".hidden-file.ext")
=> "ext"
> +> +(define (file-no-extension file)> + "Return FILE path without extension."> + (let ((without-ext (string-match "(.*)\\..*" file)))> + (if without-ext> + (regexp-substitute #f without-ext 1)> + file)))> +
Is there any particular reason why you didn't do something like:
(replace-extension file "") in file-no-extension?
> +(define (replace-extension file ext)> + "Replace file extension in FILE with EXT.> +EXT can include or exclude the beginning \".\"."> + (let ((ext (if (file-extension ext)> + (string-append "." (file-extension ext))> + ext)))> + (string-append (no-ext file) ext)))
This seems to have the same problems with hidden files. It thinks the
whole file name is an extension, so here's what happens:
(f:replace-extension ".file" "asdf")
=> ".asdf"
If you still have the time, energy and motivation, I'd greatly
appreciate a V3 addressing these issues. Feel free to discuss anything
if you need to.
>> base-commit: da553c5fcb9c3865a5e3a7506a02bb9314bb572d
On Wed, Mar 03 2021, Alexandru-Sergiu Marton wrote:
> You named the replacement function "replace-extension", but the others> start with "file-". All of them are related to file extensions. What do> you say about just removing the "file-" prefix on the first two? It> would still be clear what they do IMO, because the only thing that comes> to my mind when using a filesystem API and it has an "extension"> function is that it's related to file extensions.
Yeah, I will remove the "file-" prefix.
> Also, it seems to think that hidden files have their whole name as an> extension, and that isn't right.>> (f:file-extension ".hidden-file")> => "hidden-file"
Good catch, I didn't think about hidden files.
>> +>> +(define (file-no-extension file)>> + "Return FILE path without extension.">> + (let ((without-ext (string-match "(.*)\\..*" file)))>> + (if without-ext>> + (regexp-substitute #f without-ext 1)>> + file)))>> +>> Is there any particular reason why you didn't do something like:> (replace-extension file "") in file-no-extension?>>> +(define (replace-extension file ext)>> + "Replace file extension in FILE with EXT.>> +EXT can include or exclude the beginning \".\".">> + (let ((ext (if (file-extension ext)>> + (string-append "." (file-extension ext))>> + ext)))>> + (string-append (no-ext file) ext)))
Oops, `no-ext` should have been `file-no-extension`, I was playing
around in the REPL earlier so `no-ext` must have been some function I
renamed. My bad.
Defining `file-no-extension` using `replace-extension` would then have
caused an infinite loop.
> If you still have the time, energy and motivation, I'd greatly> appreciate a V3 addressing these issues. Feel free to discuss anything> if you need to.
Sure, a V3 should be coming.
Thanks for the review!
3 Mar 2021 14:39:54 Xinglu Chen <public@yoctocell.xyz>:
> Oops, `no-ext` should have been `file-no-extension`, I was playing> around in the REPL earlier so `no-ext` must have been some function I> renamed. My bad.
Yeah, I forgot to mention I needed to change this to make it work, it's
good that you noticed.
> Defining `file-no-extension` using `replace-extension` would then have> caused an infinite loop.
It makes sense, I didn't make this connection at that time. Thanks for
the explanation.
> Sure, a V3 should be coming.
Great! Thank you.
--
Alexandru-Sergiu Marton
[f.scm PATCH v3 0/2] Add replace-extension and extension.
I ended up not using `extension` in the definition of
`replace-extension` so it gets a separate commit.
Hidden files should now work as expected and `extension` will
return an empty string if there is no extension to be found.
Xinglu Chen (2):
Add replace-extension.
Add extension.
f.scm | 30 +++++++++++++++++++++++++++++-
1 file changed, 29 insertions(+), 1 deletion(-)
base-commit: da553c5fcb9c3865a5e3a7506a02bb9314bb572d
--
2.30.0