Clojurians Log v2
calva
@pez @brandon.ringe created PR for early feedback that closes https://github.com/BetterThanTomorrow/calva/issues/1168 here - https://github.com/BetterThanTomorrow/calva/pull/1187
Awesome, @pratikgandhi1997! Do you know how to grab the link to the VSIX built from the PR? If you post the link to the VSIX here people can easily test the feature. I’ve just quickly glanced at the code, and I like what I see. Might be able to give some more constructive feedback later tonight when I have tested and checked the code a bit closer.
What it’s about:
• Added a command toggle between implementation and test which allows you to toggle between test and the implementation. Creates a new src or test file if it’s not there.
• With this you can switch between your test and implementation with just one command.
VSIX artifact link -https://app.circleci.com/pipelines/github/BetterThanTomorrow/calva/3316/workflows/c742dbd7-79cf-42c1-ba84-b92f6bbd9265/jobs/13494/artifacts
My JS isn’t good enough to tell for sure but I don’t think the logic around main is quite right. I don’t know what testing setup Calva has but it definitely seems like having a function that, given a filepath returns the “other” filepath, and then having tests for that function would be valuable here.
I’m also wondering whether files that have src, test, and/or main as part of their namespace are going to trip this code up?
For example, test.check has a source file called test.check/src/main/clojure/clojure/test/check/clojure_test/assertions.cljc (although it does not have a corresponding test file for this namespace)
hmm, we have to check somehow that it is a maven style setup, is there any other way of doing that?
If cases like test.check are in minority, then for initial version we should be okay? fitting for every kind of folder structure would make it much more complicated =/
Consider /src/main/foo.clj and a namespace main.foo — I think you need both the namespace of the current file and the file path. That would also help you spot the /src/main/<lang>/foo/bar.cljc case since the ns would be foo.bar
(and, aside from Maven-structured projects, /src/<lang>/foo/bar.clj is a thing anyway where the test ns would be foo.bar-test and the path would be /test/<lang>/foo/bar_test.clj)
didn’t get you exactly, in tools.cli we have a path like this /src/main/clojure/clojure/tools/cli.clj and the namespace is clojure.tools.cli in this case, we can’t check if the namespace is starting with main , what exactly we can check here?
If the ns is foo.bar.quux and the file path is /src/main/clojure/foo/bar/quux.clj then /src/main/clojure is the “source path” so the most likely test path will be /src/test/clojure and the test ns will be foo.bar.quux-test so the test file will be /src/test/clojure/foo/bar/quux_test.clj
If the ns is foo.bar.quux and the file path is /src/cljs/foo/bar/quux.cljs then /src/cljs is the “source path” so the mostly likely test path will be /test/cljs and the test ns will be foo.bar.quux-test so the test file will be /test/cljs/foo/bar/quux_test.cljs.
This is why I think having tests for this would be particularly helpful.
okay, will have a look, thanks
@seancorfield I am following on your suggestion and as it doesn’t require the regex magic, it’s more cleaner and works for most cases but there is one problem I am facing, I will try to describe it:
• In tools.cli repo, the folder structure(for our purpose) looks like this,
- src
- main
- clojure
- clojure
- tools
- test
- clojure
- clojure
- tools
- clojure_legacy_test.clj /src/test/clojure/clojure/tools/cli_legacy_test.cljc and it’s namespace is clojure.tools.cli-legacy-test and there is no src for it. When I run the toggle test command it asks to create a src file at the right folder. So good so far. Now, the file cli_legacy.cljc gets created with namespace (ns main.clojure.clojure.tools.cli-legacy) and as per @pez clojure-lsp does this bit.
• Now, if i delete the original cli_legacy_test.cljc file and try to create it from clj_legacy.cljc it asks for prompt and then it creates it in the wrong folder structure(beside the outer src folder it creates a test folder). This happens because the way we create a source path is,
(current file path) - (current namespace converted file path)clj_legacy.test the source path would be only till my_path/tools.cli/src as everything else will be subtracted due to a full namespace.
I explored that if we can somehow overwrite what clojure-lsp writes to file when creating and create the same namespace with just -test appended or removed but I am not sure if that would be a good idea or if we can make it work. I can push my code or have a discussion on this if required.@pratikgandhi1997 I don’t know how to persuade LSP to do the right thing — I would have expected it to know about source paths based on deps.edn? But this whole namespace not matching the filepath thing is what I was alluding to before… @pez?
I’m not sure I completely follow, but if clojure-lsp should do something else than what it does today, I think #lsp is where to bring it. But let’s ask @ericdallo and @brandon.ringe anyway.
This is a logic done by clojure-lsp checking the source-paths (default #{src test} if not provided by user). I didn't read the whole thread but if there is something you think it should be done by clojure-lsp. please open an issue or ask on #lsp
@ericdallo I will try to summarize this:
we want to implement a feature in calva where one can run a command to switch between a test file and implementation file(if the test/src file is not there it creates one). Now, in tools.cli project, there is a file at path /src/test/clojure/clojure/tools/cli_legacy_test.cljc and it’s namespace is clojure.tools.cli-legacy-test , when we run the command, a new file gets created at the right folder but with this namespace - main.clojure.clojure.tools.cli-legacy
we want to keep the namespace similar to the module this new file gets created from(So, the namespace would be clojure.tools.cli-legacy.cljc).
Not sure, if it would be a fix in clojure-lsp or something we have to do to bypass clojure-lsp s default namespace creation or try to overwrite it
So, for this case you have a different source path right? clojure-lsp uses a default of src and test , if your project has a different source-path you need to declare that on .lsp/config.edn. like :source-paths #{"src/test" "src" "test"} I think.
Ref: https://clojure-lsp.github.io/clojure-lsp/settings/#all-settings
I think there is a issue with an idea of using deps.edn as a source to check for source-paths, but there are too many corner cases like aliases etc
yeah, that’s correct, we wanted generic capabilities in calva that works for leinignen style src and test folders + maven style folder src/main and src/test
This makes sense, if users declare the source path as Eric mentioned, I have tested that current code works out of the box as it will have the right namespace. We can probably document that, to use this feature users need to provide their source-path in ./lsp/config.edn if it’s different than the leiningen style defaults. I will refactor and push the changes mostly by tomorrow @seancorfield @pez.
Nice, @pratikgandhi1997 do you think we should add src/main and src/test to clojure-lsp defaults as well? c/c @snoe
@ericdallo The source path here should be src/main/clojure per https://github.com/clojure/tools.cli/blob/master/deps.edn#L1
(similarly the test path is src/test/clojure https://github.com/clojure/tools.cli/blob/master/deps.edn#L22 )
I see, so it does seems something specific for that project, I'm not sure this is common for other projects
This is what I referred to as <lang> in several of the examples above — and this is true of non-Maven projects as well. For example, projects that have Clojure and ClojureScript will often have src/clj, src/cljs, etc paths, where the namespace of the files should not include that leading clj, cljs portion.
This structure is very common for mixed-language projects I believe.
I see, so probably we can improve those defaults for handling projects like that
(I don’t do cljs so I’m not able to point to a specific example of that — but I’ve seen similar patterns in backend projects that mix Java and Clojure — with those top-level folders being either clj/`jvm` or clojure/`java`)
Nice! This is a shortcut I'm using all the time when coding Java in IntelliJ.
What keyboard shortcut is default for this in IntelliJ? If any. Also, maybe there is a default shortcut used by some other extension? Or maybe you have already considered that, @pratikgandhi1997?
This is a "made-up" shortcut as of now, I haven't considered intellij or any other extension for this Changing it based on intellij's shortcut seems to be a good idea
Yeah, keyboard shortcuts is the hardest part of lot of things. 😃
An alternativ could be to consolidate all test related commands under ctrlált+t, so ctrl+alt+t tab maybe for this one. Then the next question is where to move Paredit Transpose. 😃
hmm, my thinking was, currently all the test related commands are about running tests, and this functionality is similar to switching between repl output to source file, hence didn’t created it under ctrl+alt+t
let me check if ctrl+alt+t s is already used or not, could mean something like, test to source if that makes any sense :man-shrugging:
I checked out the cursive shortcut which is command+shift+t and works slightly different then this one. If your cursor is at the namespace then it will navigate between files, but if it’s at some specific function, it will try to navigate to equivalent test function/prompt to create a new function
cmd+shift+t is not used for anything super interesting, imo
We can improve this feature later to be a bit more like Cursive’s. It does sound like an improvement, right?
However, cmd+shift+t will only be for Mac, so we might want to choose something that is the same across Windows, Linux and Mac.
Currently, VSCode have this feature where it remembers where the cursor was last time the file was active, considering that it would be mostly similar in principle
but yeah, we can improve it to be like Cursive’s if needed
For IntelliJ on windows it is ctrl+shift+t. I guess Cursive has used the same shortcut as for the Java functionality.
ctrl+shift+t is used by Calva for tap> form, but we can move that elsewhere. Or at least ask in the channel if someone is attached to that. (It’s pretty new and maybe not very known.)
btw, I have yet another switch-between thing that I am planning to support. It’s for “fiddle” files. Like Rich Comments, but in separate files. So then I’d like to be able to switch between the implementation and the fiddles. Doesn’t much matter the details there, the relevance here is shortcuts. Would be nice if the switching commands where a bit intuitively linked between each other to aid the muscle memory. And we have the switch to/from output window as well.
I have some pretty strong objections to this in terms of the assumptions it makes, because it hardcodes the directory structure and file patterns.
Contrib libs all have src/main and src/test as their source and test directories — and that sort of thing is common in mixed language projects.
In addition, the use of -test on the namespace and _test on the filename is not universal — and most test runners have options so you can tell them the directory structure and namespace pattern to use, so I think this really should expose those settings from the get-go.
Folks who use the Expectations libraries tend to use -expectations and _expectations instead, for example.
And I have seen codebases that use test as a directory name in front of the last segment of the name (but this does seem rare). So you’d have src/foo/bar.clj and then test/foo/test/bar.clj for the tests.
It also doesn’t seem to support .cljc or .cljs discovery?
These all are valid points, should we allow some kind of configurations for this to user? Or is there a better way to implement this altogether?
What Cognitect’s test-runner does is to take a list of (project-root-relative) directories to search for tests and a list of patterns for namespaces to match so it will look in multiple places to find tests. That’s fine for trying to find a corresponding test for any given source file but doesn’t work going the other way so well — so that’s a very tricky use case. On the file extension, I’d say default to whatever the current file is — although some projects have multiple src folders, separating .clj, .cljc, and .cljs files so even that adds a complication. It’s a very tough problem to solve “for everyone” 😞 Do you want to create an issue on the Calva repo where we can enumerate the sorts of filename/directory patterns that appear in the wild (with links to repos that use those patterns), and then maybe figure out a subset of the possibilities that will cover “enough” cases?
My Clover config for VS Code makes some very basic assumptions that work for most of our codebase — and most simple projects — but wouldn’t work for a lot of more complex problems: this is for a task/hotkey that runs “associated” tests for the current source namespace without needing to switch files. But that doesn’t even cover everything in our own codebase at work 😕
> and most test runners have options so you can tell them the directory structure and namespace pattern to use, so I think this really should expose those settings from the get-go I agree with this. I was wondering about trouble with conventions like you mentioned.
> this is for a task/hotkey that runs “associated” tests for the current source namespace without needing to switch files.
Calva has a feature/command like this - "Run Tests for Current Namespace" - which also makes an assumption of the test namespace ending with -test , I think. Maybe it's something to keep in mind when planning and implementing this switch feature: if we make the check for the test namespace/file there check for more possibilities, maybe we can use some of that code with that namespace test command, or change it to be similar.
We should collect thoughts here: https://github.com/BetterThanTomorrow/calva/issues/1168
@brandon.ringe Perfect! I’ll set aside some time this afternoon to write up some notes on that.
Awesome. I just mentioned your point about different conventions, but please elaborate there if needed.