Hi, I did the changes for the review. It took a while (sorry for that) but it got cleaner as a result (thank you!) Firstoff: I’d like to assign my copyright to the FSF. I’ll sign the papers once I receive them. Also I have an Employer disclaimer of rights on paper for Emacs already, so that should not cause trouble. Attached is a new squashed patch. I’ll send another email with only the commits for the individual changes on top of the original squashed patch to avoid tripping up tools that extract diffs. Changes: - [X] LGPL - [X] Please add the new files to the relevant ‘Makefile.am’ files. - [X] Note the changes in Makefiles in the commit - [X] Please capitalize “Scheme” and “Wisp” (in general we like to pay attention to typography, spelling, and language in the manual.) - [X] s/(wisp)/, also referred to as @dfn{Wisp} (for ``Whitespace …'')/ - [X] Two spaces after end-of-sentence periods, to facilitate navigation in Emacs. - [X] indent “the usual way” - [X] comments always with ;; except for margin comments - [X] (read-enable 'curly-infix) This needs to be: (eval-when (expand load eval) (read-enable 'curly-infix)) - [X] Please make them #:use-module clauses in the ‘define-module’ form - [X] I’d encourage following the usual naming convention, so ‘in-indent?’, ‘in-comment?’, etc. - [X] use exception objects or SRFI-35 error conditions instead of throw with symbols - [X] add a test for source location info A change I did not do: - [ ] +Use record-types for the lines+. Reason: I decided not to do this because it currently needs to propagate the source properties when retrieving the code, so this is not a good match for a record type (it may become one with an annotated reader, but I’d like to shift that to a later change: > implementing the “annotated read” interface (info "(guile) > Annotated Scheme Read"). That can come in a subsequent patch, though. Ludovic Courtès writes: > Overall I’m confident the code has been battle-tested (I suppose there > are minimal changes compared to Wisp, right?), so I’ll comment mostly on > cosmetic issues. Yes, the code comes directly from Wisp. The original was simply auto-translated. Naturally that’s no guarantee that there are no bugs, but there should be few enough that this can be used in production. >> From c7a50f632293cf88f241d45d1fd52fa2f58ce772 Mon Sep 17 00:00:00 2001 >> From: Arne Babenhauserheide >> Date: Fri, 3 Feb 2023 22:20:04 +0100 >> Subject: [PATCH] Add language/wisp, wisp tests, and srfi-119 documentation >> >> * doc/ref/srfi-modules.texi (srfi-119): add node >> * module/language/wisp.scm: New file. >> * module/language/wisp/spec.scm: New file. >> * test-suite/tests/srfi-119.test: New file. > > Please add the new files to the relevant ‘Makefile.am’ files. Added. >> +* SRFI-119:: Wisp: simpler indentation-sensitive scheme. > > Please capitalize “Scheme” (in general we like to pay attention to > typography, spelling, and language in the manual.) > >> +The languages shipped in Guile include SRFI-119 (wisp), an encoding of > > s/(wisp)/, also referred to as @dfn{Wisp} (for ``Whitespace …'')/ > >> +Scheme that allows replacing parentheses with equivalent indentation and >> +inline colons. See > > Two spaces after end-of-sentence periods, to facilitate navigation in > Emacs. Adjusted. >> +(define-module (language wisp) >> + #:export (wisp-scheme-read-chunk wisp-scheme-read-all >> + wisp-scheme-read-file-chunk wisp-scheme-read-file >> + wisp-scheme-read-string)) > > Please remove tabs from this file and indent it “the usual way”. You > can do that by passing it through Emacs and using ‘M-x indent-region’, > or possibly using ‘guix style -f’. I used M-x indent-region. > Use two leading colons for comments, except for margin comments. 👍 >> +(read-enable 'curly-infix) > > This needs to be: > > (eval-when (expand load eval) > (read-enable 'curly-infix)) Adjusted — thank you (when to use eval-when was always a bit of a mystery to me …). > Please make them #:use-module clauses in the ‘define-module’ form above. Done. > Instead of using lists or pairs for “lines”, I suggest defining a proper > record type, like: > … > This will make the code easier to read and type-clear. Instead of this, I created a make-line procedure to make this easy to refactor later when the code no longer needs adaptions when retrieving the code part. > I’d encourage following the usual naming convention, so ‘in-indent?’, > ‘in-comment?’, etc. Done. >> + ((and inunderscoreindent (and (not (equal? #\space next-char)) >> (not (equal? #\newline next-char)))) >> + (throw 'wisp-syntax-error "initial underscores without following >> whitespace at beginning of the line after" (last >> indent-and-symbols))) > > I suggest using exception objects or SRFI-35 error conditions (they’re > interchangeable) carrying information about the king of parsing error > and its location. That way, the user interface, such as the compiler, > can produce helpful error messages (and internationalized, too). Done: I used the convenience transformation, i.e. (raise-exception (make-exception-from-throw 'wisp-syntax-error (list (format #f "Level ~A not found in the indentation-levels ~A." level indentation-levels)))) >> +++ b/module/language/wisp/spec.scm > Can you make it LGPLv3+? It’s a small file anyway. I investigated and found that it actually was LGPLv3+ in the beginning and I got permission to relicense to MIT for the SRFI, so with permission of Maxime (who did later changes) this is now LGPLv3+ again. >> + (let ((m (make-fresh-user-module))) >> + ;; Provide a separate `current-reader' fluid so that >> + ;; compile-time changes to `current-reader' are >> + ;; limited to the current compilation unit. >> + (module-define! m 'current-reader (make-fluid)) >> + ;; Default to `simple-format', as is the case until >> + ;; (ice-9 format) is loaded. This allows >> + ;; compile-time warnings to be emitted when using >> + ;; unsupported options. >> + (module-set! m 'format simple-format) > > I’d remove this last statement, I think we should avoid fiddling with > bindings here. Removed it now. >> +(define-module (test-srfi-119) >> + #:use-module (test-suite lib) >> + #:use-module (srfi srfi-1) >> + #:use-module (language wisp)) > > The test suite looks rather short; it would be nice if it would cover > more code. I have a pretty extensive test-suite in wisp itself but I didn’t think it proper to just paste it into Guile. If you’d prefer that, I can still add it all, though. > In particular, I think source location info is important for front-ends > like this, because it determines the quality of error messages and thus > its ease of use. Perhaps you could add test in that direction? I added a test and found shortcomings thanks to it. After fixing these, the current code has more robust source-property handling (that hadn’t yet been tested further than ensuring that error messages give proper source positions — which they did). > Overall I feel we should be able to merge this rather soon. Do ping me > or Andy when needed! Thank you! The following is the complete squashed patch.