Fix CI failures

[rrudakov]

• Looks like project-root function was introduced in Emacs 28.1. So tests fail on Emacs 27.
• Linter complains about duplicated square brackets in the regex "^[] \"'><,;|&{()[@\^]"`. I'm not sure it's an error, maybe one of the pairs should be escaped?

---

Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

• The commits are consistent with our contribution guidelines
• The new code is not generating bytecode or M-x checkdoc warnings
• You've updated the changelog (if adding/changing user-visible functionality)
• You've updated the readme (if adding/changing user-visible functionality)

Thanks!

[rrudakov]

@bbatsov what's the best way to trigger CI?

[bbatsov]

I'm guess for you'd it be easiest to just push changes here. I'm not sure you can run jobs directly from the console https://app.circleci.com/pipelines/github/clojure-emacs/inf-clojure

[bbatsov]

Linter complains about duplicated square brackets in the regex "^[] "'><,;|&{()[@^]"`. I'm not sure it's an error, maybe one of the pairs should be escaped?

Looking more closely at this code - it doesn't even seem that's a regexp (it's passed only to skip-chars-backward whose arg is not a regexp). But for some reason that's confusing the regexp linter.

[rrudakov]

I'm guess for you'd it be easiest to just push changes here.

That was my assumption, but nothing happened. Maybe I should convert it to non-draft?

[bbatsov]

Looks like project-root function was introduced in Emacs 28.1. So tests fail on Emacs 27.

I see. I guess we can require Emacs 28. It's fairly old by now anyways.

[rrudakov]

One of the failing tests is:

(it "computes no bounds for point directly after a break expression"
    (ict-with-assess-buffers
     ((a (insert "@")))
     (with-current-buffer a
       (expect
        (ict-bounds-string (inf-clojure-completion-bounds-of-expr-at-point))
        :not :to-be nil))))

I honestly don't really understand what it's about and Git history doesn't give me much details either. Any tips would be welcome :)

[bbatsov]

That was my assumption, but nothing happened. Maybe I should convert it to non-draft?

I guess so.

I honestly don't really understand what it's about and Git history doesn't give me much details either. Any tips would be welcome :)

This is trying to extract the string part from some form (e.g. from @foo it'd be foo) for completion purposes. Here it should be nil as there's nothing after @. It's related to this regexp that's not really a regexp we discussed earlier.

[rrudakov]

This is trying to extract the string part from some form (e.g. from @foo it'd be foo) for completion purposes. Here it should be nil as there's nothing after @. It's related to this regexp that's not really a regexp we discussed earlier.

well, test expects it to be non-nil for some reason, but (inf-clojure-completion-bounds-of-expr-at-point) returns nil. So ict-bounds-string signals an error, because buffer-substring is called with nil nil arguments.

[bbatsov]

Ah, probably it's actually supposed to be empty string when there are no relevant characters.

[rrudakov]

Looking at the code I would assume that returning nil from this function is OK. It's used in 2 places and the result is always checked for nil. Also it returns a cons of points, not a string. Maybe the test is incorrect after all?

(defun inf-clojure-completion-bounds-of-expr-at-point ()
  "Return bounds of expression at point to complete."
  (when (not (memq (char-syntax (following-char)) '(?w ?_)))
    (save-excursion
      (let* ((end (point))
             (skipped-back (skip-chars-backward inf-clojure-clojure-expr-break-chars))
             (start (+ end skipped-back))
             (chars (or (thing-at-point 'symbol)
                        (inf-clojure--kw-to-symbol (buffer-substring start end)))))
        (when (> (length chars) 0)
          (let ((first-char (substring-no-properties chars 0 1)))
            (when (string-match-p "[^0-9]" first-char)
              (cons (point) end))))))))

(defun inf-clojure-completion-expr-at-point ()
  "Return expression at point to complete."
  (let ((bounds (inf-clojure-completion-bounds-of-expr-at-point)))
    (and bounds
         (buffer-substring (car bounds) (cdr bounds)))))

(defun inf-clojure-completion-at-point ()
  "Retrieve the list of completions and prompt the user.
Returns the selected completion or nil."
  (let ((bounds (inf-clojure-completion-bounds-of-expr-at-point)))
    (when (and bounds (inf-clojure-get-feature (inf-clojure-proc) 'completion 'no-error))
      (list (car bounds) (cdr bounds)
            (if (fboundp 'completion-table-with-cache)
                (completion-table-with-cache #'inf-clojure-completions)
              (completion-table-dynamic #'inf-clojure-completions))))))

[rrudakov]

Looks like project-root function was introduced in Emacs 28.1. So tests fail on Emacs 27.

I see. I guess we can require Emacs 28. It's fairly old by now anyways.

Should I remove Emacs 27 from CI config?

[bbatsov]

I'm fine with your proposal on both points.

[bbatsov]

One more thing - seems the CI wasn't running for PRs from forks. Should be working properly now. (after you next change)

[rrudakov]

Sorry for the noise again. I've done some digging in the Git history and found out that this test was introduced in this PR: #129

The original regex was

" \t\n\"\'`><,;|&{()[]@\\^"

(commit 74a6fac). If I change the regex to this, the test passes, it returns the boundaries of @ character.

Then in another commit (dbe2839) it was changed to the current value and this test stopped working. I'm a bit hesitant to change the regex. Maybe we should use rx to simplify it?

[bbatsov]

I don't think rx would help, as this is not a regexp ( the docstring is wrong) - the only usage is here

(skipped-back (skip-chars-backward inf-clojure-clojure-expr-break-chars))

and this function takes a list of chars (a string). I guess we can update the tests and the wrong var description for and call it a day.

[rrudakov]

I don't think rx would help, as this is not a regexp ( the docstring is wrong) - the only usage is here

(skipped-back (skip-chars-backward inf-clojure-clojure-expr-break-chars))

and this function takes a list of chars (a string). I guess we can update the tests and the wrong var description for and call it a day.

Haha, that's what I've just figured out as well. But from the original PR I assume it should match @, and currently it doesn't. I'll try to make it work.

[rrudakov]

I decided to update the test to expect nil. The string, expected by skip-chars-backward is not as simple as it seems:

STRING is like the inside of a ‘[...]’ in a regular expression
except that ‘]’ is never special and ‘\’ quotes ‘^’, ‘-’ or ‘\’
 (but not at the end of a range; quoting is never needed there).
Thus, with arg "a-zA-Z", this skips letters stopping before first nonletter.
With arg "^a-zA-Z", skips nonletters stopping before first letter.
Char classes, e.g. ‘[:alpha:]’, are supported.

Returns the distance traveled, either zero or positive.

Moving characters around breaks tests, so there is still warning about duplicated brakets and I don't really know how to fix it, because it looks like a valid case to have one "special" ] and one non-special in the same string.

All the tests are fixed now, @bbatsov it's ready for review.

[rrudakov]

This linter is stupid :) If I remove duplicated brackets tests actually pass, but I get another linter error:

Running linter ‘re’
File ‘assess-robot-autoloads.el’: no warnings
inf-clojure.el:1510:50-69: error: In inf-clojure-clojure-expr-break-chars: Unterminated character alternative (pos 1..18)
  "^[] \"'`><,;|&{()@\\^"
Found 1 warning in file ‘inf-clojure.el’

I guess it tries to lint it as an actual regex, but it's not aware that characters in this string have special meaning.

[bbatsov]

I guess it tries to lint it as an actual regex, but it's not aware that characters in this string have special meaning.

I made the same discovery earlier today when I tried to fix this myself. Not sure how to instruct the linter to ignore this, but we'll figure it out later.