Improved alias detection for cljr--ns-alias-at-point

[dgtized]

Addresses https://github.com/clojure-emacs/clj-refactor.el/issues/524.

Prevously ns aliases could include quasiquote, sharp-quote and was not identifying dotted aliases. After reading through the docs on valid namespace alias, it appears aliases can be any valid symbol. I tested briefly, and all of the following namespace aliases are valid in both Clojure and Clojurescript.

foo.bar or foo.bar.baz test? or ?test or a?b a:b (a::b is legal in cljs only) tl'an or name' l33t or a1 foo$bar

I suspect that clojure-mode would benefit from providing a robust clojure-symbol-at-point which could be re-used here. It has a robust symbol regexp in clojure--sym-regexp, which could then be trimmed of leading : and such, but that is internal to clojure-mode, so I thought it better if it could be exposed there before using it here.

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

• [x] The commits are consistent with our contribution guidelines
• [x] You've added tests (if possible) to cover your change(s)
• [x] The new code is not generating byte compile warnings (run cask exec emacs -batch -Q -L . -eval "(progn (setq byte-compile-error-on-warn t) (batch-byte-compile))" clj-refactor.el)
• [x] All tests are passing (run ./run-tests.sh)
• [x] You've updated the changelog (if adding/changing user-visible functionality)
• [x] You've updated the readme (if adding/changing user-visible functionality)

Thanks!

[vemv]

Looking good!

Let me know when it's ready.

[dgtized]

There is one test failing from the integration tests and it's:

  Scenario: Require is inserted automagically for namespaced keyword
    When I insert:
    """
    (ns cljr.core)

    (::util)
    """
    And the cache of namespace aliases is populated
    And I place the cursor after "util"
    And I start an action chain
    And I type "/"
    And I type "refactor-nrepl.util"
    And I press "RET"
    And I type "some-keyword"
    And I execute the action chain
    Then I should see:
    """
    (ns cljr.core
      (:require [refactor-nrepl.util :as util]))

    (::util/some-keyword)
    """

I'm not quite sure how to debug this example. I presume the expectation here is that since we have previously used the ::util, the use of the slash should prompt for refactor-nrepl.util and then insert the require. However, this PR is still correctly removing the :: from ::util, as that's already accounted for in the existing unit tests. I'm having difficulty debugging why it's failing for the integration (because the integration tests are kind of opaque). I suspect the correct approach is to rewrite the integration test as a unit test, and work my way through it but haven't had a chance just yet as I just noticed the failing integration test. I'll change this from a draft once I've addressed this unless someone else has a suggestion here.

[vemv]

Unit tests are the only thing we run (see Makefile). It seems unlikely that we'll salvage the integration ones.

Anyway that the PR change the result of said test, or was it broken already?

[dgtized]

I switched to my laptop, and integration tests pass both with and without this test. On my desktop, integration would pass on the main branch but fail on this one. Both laptop and desktop pass unit tests locally. Not sure how my local environment is changing the result there, but it would be helpful if someone else could run the test.

One other note, when I've been running the unit-tests (which pass on both laptop and desktop, before and after this branch), I've been using: CI=1 cask exec buttercup -l clj-refactor.el -L ., as running CI=1 make unit-tests does not specifically select the clj-refactor.el inside the git repo and appears to be using a system version somehow. I'm not familiar with how cask does environment isolation for tests, so would welcome any suggestions there.

[vemv]

You can have your fork run by CircleCI btw!

[vemv]

Thanks much for this one!

I think the build will just pass. For the remaining PR perhaps you can attach a screenshot of a green Circle build - it's something I do when contributing in various places.

[dgtized]

How do I trigger a circleci test build for a PR on a fork?

[vemv]

You have to manually add your project to Circle just like any other one

btw the build failed :( I was wrong, we do trigger integration tests.

It's the same failure you had perceived:

https://app.circleci.com/pipelines/github/clojure-emacs/clj-refactor.el/124/workflows/62577b8f-fc8f-4a27-8f0f-384462f983fd/jobs/377

[dgtized]

Apologies on that, that's why I was requesting someone else run it as I wasn't sure. Not sure how you want to proceed. As I mentioned above, there is an explicit test ensuring that the output of cljr--ns-alias-at-point will still return util from ::util/ at point, which was the only function changed. I also have had zero luck modifying any of the integration tests as it's not clear where they fail. I've tested locally and if I type ::set/ it inserts clojure.set :as set as a libspec and proceeds. It also prompts if I add clojure.edn :as set in another namespace.

Do you want to revert this change, disable the failing spec, or add a unit test verifying the behavior, or some other solution or a combination of the above?

[vemv]

I'd suggest fixing the failing spec and opening a PR for that one. https://github.com/clojure-emacs/clj-refactor.el/commits/master shows that it was this commit that introduced the failure.

I'm not fond of our integration tests and am just about as knowledgeable as you in debugging them :(

It might not be that hard with some guesswork.

LMK how it goes, can adapt plans depending on your input.

Thanks - V