| Message ID | 86b13fd4916ffecb1947d0879805a6d0c32542bf.1706386650.git.liliana.prikler@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#68412,v2] scripts: edit: Accept generic formatting parameter. | expand |
Hi, On sam., 13 janv. 2024 at 00:35, Liliana Marie Prikler <liliana.prikler@gmail.com> wrote: > +@vindex GUIX_EDITOR_LOCATION_FORMAT > +The default convention used by @code{guix edit} when invoking > +@code{$EDITOR} is to pass it @code{+@var{line} @var{file}} to open > +@var{file} at the given @var{line}. > +You can change this convention for editors that do not support it > +by setting @env{GUIX_EDITOR_LOCATION_FORMAT}. > +For instance, to set things up with kate, use: > + > +@example > +export VISUAL=kate > +export GUIX_EDITOR_LOCATION_FORMAT='--line=$@{LINE@} $@{FILE@}' > +# Assume you want to hack on kate > +guix edit kate > +@end example First, it appears to me inconsistent to speak about EDITOR and then to use VISUAL in the example. I suggest to have: The default convention used by @code{guix edit} when invoking @code{$EDITOR} or @code{VISUAL} is to pass it @code{+@var{line} @var{file}} to open and the same example. Or change the example and replace with: export EDITOR=kate export GUIX_EDITOR_LOCATION_FORMAT='--line=$@{LINE@} $@{FILE@}' Second, I think that using markers that can be interpreted by Bash shell can lead to confusion. For instance, $ LINE=foo; FILE=bar # somewhere in my config for whatever reasons then: --8<---------------cut here---------------start------------->8--- $ export GUIX_EDITOR_LOCATION_FORMAT='--line=${LINE} ${FILE}' $ echo $GUIX_EDITOR_LOCATION_FORMAT --line=${LINE} ${FILE} $ export GUIX_EDITOR_LOCATION_FORMAT="--line=${LINE} ${FILE}" $ echo $GUIX_EDITOR_LOCATION_FORMAT --line=foo bar --8<---------------cut here---------------end--------------->8--- Well, simple quote versus double quote appears to me subtle. Since it is an hard text replacement, why not remove $ and just have the placeholder {LINE} or {FILE}? Or <LINE> and <FILE>? Or whatever that is not interpreted by common shells. > +Note, that Guix only matches the literal strings @code{$@{LINE@}} and > +@code{$@{FILE@}} here. These may look like shell parameters, but their > +short form is currently not supported. Therefore, it would make that more clear or even obsolete. Cheers, simon
Hi, Liliana Marie Prikler <liliana.prikler@gmail.com> skribis: > This will hopefully end the opening of unwanted files. > > * guix/scripts/edit.scm (%location-format): New parameter. > (location->location-specification): Use %location-format. > (spawn-editor): Adjust accordingly. > > Fixes: Pass special flags to ‘kate’ <https://bugs.gnu.org/44272#14> LGTM! >> > Fixes: Pass special flags to ‘kate’ <https://bugs.gnu.org/44272#14> >> >> Rather: “Fixes <https://issues.guix.gnu.org/44272>.” > I'm using a convention that I've proposed earlier in [1]. > Since we're currently adding ChangeIds without any of the supported > infra (AFAIK), I think following my own proposal here is fair game. It’s not: conventions, by definition, are agreed upon collectively. Regardless of the merits of a proposal, we first have to build consensus for the proposal before starting using it. Thanks, Ludo’.
Hi,
On Mon, 29 Jan 2024 at 14:24, Ludovic Courtès <ludo@gnu.org> wrote:
> LGTM!
This does not LGTM for the reason I invoked earlier: single-quote
versus double-quote and the interpretation of ${LINE}.
I think it would be less confusing to have another placeholder, as
just {LINE} or whatever else.
For what my opinion is worth.
Cheers,
simon
Am Montag, dem 29.01.2024 um 12:10 +0100 schrieb Simon Tournier: > Hi, > > On sam., 13 janv. 2024 at 00:35, Liliana Marie Prikler > <liliana.prikler@gmail.com> wrote: > > > +@vindex GUIX_EDITOR_LOCATION_FORMAT > > +The default convention used by @code{guix edit} when invoking > > +@code{$EDITOR} is to pass it @code{+@var{line} @var{file}} to open > > +@var{file} at the given @var{line}. > > +You can change this convention for editors that do not support it > > +by setting @env{GUIX_EDITOR_LOCATION_FORMAT}. > > +For instance, to set things up with kate, use: > > + > > +@example > > +export VISUAL=kate > > +export GUIX_EDITOR_LOCATION_FORMAT='--line=$@{LINE@} $@{FILE@}' > > +# Assume you want to hack on kate > > +guix edit kate > > +@end example > > First, it appears to me inconsistent to speak about EDITOR and then > to use VISUAL in the example. I suggest to have: > > The default convention used by @code{guix edit} when invoking > @code{$EDITOR} or @code{VISUAL} is to pass it @code{+@var{line} > @var{file}} to open > > and the same example. Or change the example and replace with: > > export EDITOR=kate > export GUIX_EDITOR_LOCATION_FORMAT='--line=$@{LINE@} $@{FILE@}' I think "or VISUAL" would be acceptable, but repeating what we wrote earlier. I wouldn't set EDITOR=kate, because it is graphical after all. > Second, I think that using markers that can be interpreted by Bash > shell can lead to confusion. For instance, > > $ LINE=foo; FILE=bar # somewhere in my config for whatever > reasons > > then: > > --8<---------------cut here---------------start------------->8--- > $ export GUIX_EDITOR_LOCATION_FORMAT='--line=${LINE} ${FILE}' > $ echo $GUIX_EDITOR_LOCATION_FORMAT > --line=${LINE} ${FILE} > > $ export GUIX_EDITOR_LOCATION_FORMAT="--line=${LINE} ${FILE}" > $ echo $GUIX_EDITOR_LOCATION_FORMAT > --line=foo bar > --8<---------------cut here---------------end--------------->8--- > > Well, simple quote versus double quote appears to me subtle. > > Since it is an hard text replacement, why not remove $ and just have > the placeholder {LINE} or {FILE}? Or <LINE> and <FILE>? Or whatever > that is not interpreted by common shells. Because it is only a hard text replacement *for now*. We might find that there's merit to having gash interpret these later on. I know there's like fifty conventions for formatting strings and we have to pick one, but I'd like to think that this isn't just a pointless exercise in forward compatibility. > > +Note, that Guix only matches the literal strings @code{$@{LINE@}} > > and > > +@code{$@{FILE@}} here. These may look like shell parameters, but > > their > > +short form is currently not supported. > > Therefore, it would make that more clear or even obsolete. /me hints at "currently" Cheers
Hi, Liliana Marie Prikler <liliana.prikler@gmail.com> skribis: >> Second, I think that using markers that can be interpreted by Bash >> shell can lead to confusion. For instance, [...] >> $ export GUIX_EDITOR_LOCATION_FORMAT="--line=${LINE} ${FILE}" >> $ echo $GUIX_EDITOR_LOCATION_FORMAT >> --line=foo bar >> --8<---------------cut here---------------end--------------->8--- >> >> Well, simple quote versus double quote appears to me subtle. >> >> Since it is an hard text replacement, why not remove $ and just have >> the placeholder {LINE} or {FILE}? Or <LINE> and <FILE>? Or whatever >> that is not interpreted by common shells. > Because it is only a hard text replacement *for now*. We might find > that there's merit to having gash interpret these later on. I know > there's like fifty conventions for formatting strings and we have to > pick one, but I'd like to think that this isn't just a pointless > exercise in forward compatibility. It’s true that someone not well versed in shell or someone distracted could easily find themselves having ${LINE} and ${FILE} shell-expanded even though we precisely don’t want that. One way out would be to use a different syntax, say, %LINE% and %FILE%. With the syntax clearly different from shell variables, we’d avoid those easy mistakes. WDYT? Ludo’.
Am Mittwoch, dem 07.02.2024 um 23:22 +0100 schrieb Ludovic Courtès: > Hi, > > Liliana Marie Prikler <liliana.prikler@gmail.com> skribis: > > > > Second, I think that using markers that can be interpreted by > > > Bash > > > shell can lead to confusion. For instance, > > [...] > > > > $ export GUIX_EDITOR_LOCATION_FORMAT="--line=${LINE} ${FILE}" > > > $ echo $GUIX_EDITOR_LOCATION_FORMAT > > > --line=foo bar > > > --8<---------------cut here---------------end--------------->8--- > > > > > > Well, simple quote versus double quote appears to me subtle. > > > > > > Since it is an hard text replacement, why not remove $ and just > > > have the placeholder {LINE} or {FILE}? Or <LINE> and <FILE>? Or > > > whatever that is not interpreted by common shells. > > Because it is only a hard text replacement *for now*. We might > > find that there's merit to having gash interpret these later on. I > > know there's like fifty conventions for formatting strings and we > > have to pick one, but I'd like to think that this isn't just a > > pointless exercise in forward compatibility. > > It’s true that someone not well versed in shell or someone distracted > could easily find themselves having ${LINE} and ${FILE} shell- > expanded even though we precisely don’t want that. > > One way out would be to use a different syntax, say, %LINE% and > %FILE%. With the syntax clearly different from shell variables, we’d > avoid those easy mistakes. > > WDYT? Well, I said my opinion already in reply to Simon, but if y'all strongly feel that preventing this confusion is preferable and can agree to a common syntax, by all means go ahead and commit it with that change. I do think there's value in having this interpretable by gash at some point, but maybe I'm thinking too far into the future. Cheers
Hi, Liliana Marie Prikler <liliana.prikler@gmail.com> skribis: > Well, I said my opinion already in reply to Simon, but if y'all > strongly feel that preventing this confusion is preferable and can > agree to a common syntax, by all means go ahead and commit it with that > change. Conventionally the submitter would push the patches past the final line. If you agree with the proposal, please go ahead; if you don’t, we can discuss it further, though I think the discussion should be proportional to the stakes. (Personally I agree there’s a risk of confusion like Simon noted but I’m fine either way.) Thanks, Ludo’.
Hi, On Sat, 10 Feb 2024 at 10:58, Ludovic Courtès <ludo@gnu.org> wrote: > Conventionally the submitter would push the patches past the final line. > If you agree with the proposal, please go ahead; if you don’t, we can > discuss it further, though I think the discussion should be proportional > to the stakes. (Personally I agree there’s a risk of confusion like > Simon noted but I’m fine either way.) I agree with this paragraph. 1. I think that the current placeholder can be confusing (quote vs double-quote). 2. The envisioned future feature with Gash is not clear for me. So I do not know what would be better. 3. I do not have a strong opinion and I can live with whatever choice -- although I would live better if there is no confusion. ;-) I propose {LINE} as placeholder because in case of #2 it would easy to support both {LINE} and ${LINE} then reducing some backward compatibilities issue. WDYT? Cheers, simon
Am Samstag, dem 10.02.2024 um 12:01 +0100 schrieb Simon Tournier: > Hi, > > On Sat, 10 Feb 2024 at 10:58, Ludovic Courtès <ludo@gnu.org> wrote: > > > Conventionally the submitter would push the patches past the final > > line. If you agree with the proposal, please go ahead; if you > > don’t, we can discuss it further, though I think the discussion > > should be proportional to the stakes. (Personally I agree there’s > > a risk of confusion like Simon noted but I’m fine either way.) > > I agree with this paragraph. > > 1. I think that the current placeholder can be confusing (quote vs > double-quote). Is this something we can fix by pointing out the single quotes, or is it better not to try that? > 2. The envisioned future feature with Gash is not clear for me. So > I do not know what would be better. To make it a little clearer, we currently have more or less implementation-defined behaviour through calling system with a string- join'ed command. (This is not the best way of invoking an editor, but it works, and it also works with EDITORs like "emacsclient -c" if your shell is Bash – which Guix System has by default.) If we were to pull in Gash, we could process the command line a priori and then use system* or invoke. > 3. I do not have a strong opinion and I can live with whatever choice > -- although I would live better if there is no confusion. ;-) > > I propose {LINE} as placeholder because in case of #2 it would easy > to support both {LINE} and ${LINE} then reducing some backward > compatibilities issue. What would be the way forward if we use {LINE} now? Cheers
Hi, On mar., 13 févr. 2024 at 16:04, Liliana Marie Prikler <liliana.prikler@gmail.com> wrote: >> 1. I think that the current placeholder can be confusing (quote vs >> double-quote). > > Is this something we can fix by pointing out the single quotes, or is > it better not to try that? Well, even if it would be using Gash, the issue quote/double-quote would be still there because it is too late – it does not depend on Guix internals but only user Shell script. If you are envisioning the user Shell would be Gash, well for what it is worth, I am not convinced that – as an user – I would switch; I will still use Bash, almost surely. ;-) Therefore, GUIX_EDITOR_LOCATION_FORMAT needs to support Bash compatible syntax. And thus, the placeholder will stay – at least for backward compatibility. I propose {LINE} because it seems familiar with ${LINE}. Or I proposed <LINE>. Ludo proposes %LINE%. Last, I am not sure to understand the idea behind Gash. And if is something about Guix internals, then the best will be to have an internal replacement from ’PLACEHOLDER’ to ’${PLACEHOLDER}’ and not to have something that the user Shell can interpret. All in all, IMHO, let pick one of them: {LINE} <LINE> %LINE% :-) Cheers, simon
diff --git a/doc/guix.texi b/doc/guix.texi index c458befb76..2ae3871464 100644 --- a/doc/guix.texi +++ b/doc/guix.texi @@ -13989,6 +13989,35 @@ Invoking guix edit @var{directory}}) allows you to add @var{directory} to the front of the package module search path and so make your own packages visible. +@vindex GUIX_EDITOR_LOCATION_FORMAT +The default convention used by @code{guix edit} when invoking +@code{$EDITOR} is to pass it @code{+@var{line} @var{file}} to open +@var{file} at the given @var{line}. +You can change this convention for editors that do not support it +by setting @env{GUIX_EDITOR_LOCATION_FORMAT}. +For instance, to set things up with kate, use: + +@example +export VISUAL=kate +export GUIX_EDITOR_LOCATION_FORMAT='--line=$@{LINE@} $@{FILE@}' +# Assume you want to hack on kate +guix edit kate +@end example + +Alternatively, for gnome-text-editor, which has no such flag, simply +skip it: + +@example +export VISUAL=gnome-text-editor +export GUIX_EDITOR_LOCATION_FORMAT='$@{FILE@}' +# Assume you want to hack on gnome +guix edit gnome +@end example + +Note, that Guix only matches the literal strings @code{$@{LINE@}} and +@code{$@{FILE@}} here. These may look like shell parameters, but their +short form is currently not supported. + @node Invoking guix download @section Invoking @command{guix download} diff --git a/guix/scripts/edit.scm b/guix/scripts/edit.scm index b7b4cd2514..130470dbc1 100644 --- a/guix/scripts/edit.scm +++ b/guix/scripts/edit.scm @@ -25,6 +25,7 @@ (define-module (guix scripts edit) #:use-module ((guix diagnostics) #:select (location-file location-line)) #:use-module (gnu packages) + #:use-module (ice-9 string-fun) #:use-module (srfi srfi-1) #:use-module (srfi srfi-37) #:export (%editor @@ -62,6 +63,10 @@ (define %editor ;; For development, user can set custom value for $EDITOR. (make-parameter (or (getenv "VISUAL") (getenv "EDITOR") "nano"))) +(define %location-format + (make-parameter (or (getenv "GUIX_EDITOR_LOCATION_FORMAT") + "+${LINE} ${FILE}"))) + (define (search-path* path file) "Like 'search-path' but exit if FILE is not found." (let ((absolute-file-name (or (search-path path file) @@ -78,18 +83,21 @@ (define (search-path* path file) (define (location->location-specification location) "Return the location specification for LOCATION for a typical editor command line." - (list (string-append "+" - (number->string - (location-line location))) - (search-path* %load-path (location-file location)))) + (let* ((spec (%location-format)) + (spec (string-replace-substring + spec "${LINE}" + (number->string (location-line location)))) + (spec (string-replace-substring + spec "${FILE}" + (search-path* %load-path (location-file location))))) + spec)) (define (spawn-editor locations) "Spawn (%editor) to edit the code at LOCATIONS, a list of <location> records, and exit." (catch 'system-error (lambda () - (let ((file-names (append-map location->location-specification - locations))) + (let ((file-names (map location->location-specification locations))) ;; Use `system' instead of `exec' in order to sanely handle ;; possible command line arguments in %EDITOR. (exit (system (string-join (cons (%editor) file-names))))))