-
Notifications
You must be signed in to change notification settings - Fork 352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Smoother main and test discovery #2532
Conversation
metals/src/main/scala/scala/meta/internal/metals/Messages.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/debug/DebugProvider.scala
Outdated
Show resolved
Hide resolved
3a40326
to
0c53957
Compare
metals/src/main/scala/scala/meta/internal/metals/Messages.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/debug/DebugProvider.scala
Outdated
Show resolved
Hide resolved
073bb5e
to
20267c0
Compare
Now it's possible to just give a `runType` and a `path` argument when calling `debug-adapter-start`. The possible `runType`s are `run`, `testFile` and `testTarget`.
b6332fd
to
042b6b8
Compare
Alright, I think this should now be ready to review. I ended up keeping the separate params mainly because I think it is way cleaner than accounting for everything being nullable. I tried that and it got messy pretty quickly. Plus, this allows for clients to start to use this without worrying at all about any existing implementation or things breaking as the others work fully the same. I figured out the tests, and it was just a timing thing, and I need to wait a bit to ensure that the classes are cached and then the tests work. However, for windows I had major issues getting the URI right. Finding a clean way to always send in the correct URI proved to be way harder than anticipated and I kept getting a mixture of I think this is a good foundation to start actually using the discovery and then fine tuning. Let me know what you think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for digging into the failures! That was a lot of debugging from what I've seen in the emails 😨
metals/src/main/scala/scala/meta/internal/metals/debug/DebugProvider.scala
Outdated
Show resolved
Hide resolved
Woo, all ✅ on the window test! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks to @ckipp01 it's now possible to run test or main classes inside the current file. The Metals PR that added this functionality is scalameta/metals#2532
Thanks to @ckipp01 it's now possible to run test or main classes inside the current file. The Metals PR that added this functionality is scalameta/metals#2532
Thanks to @ckipp01 it's now possible to run test or main classes inside the current file. The Metals PR that added this functionality is scalameta/metals#2532
Thanks to @ckipp01 it's now possible to run test or main classes inside the current file. The Metals PR that added this functionality is scalameta/metals#2532
What this is
This pr introduces some new functionality around run/test/debug. My main goal with this is to enable clients to quickly be able to run or test based off of where they are, and that's it. This is especially useful for users without code lenses or useful in the case where a user doesn't want to go back to the file with the main in to hit run, or scroll all the way back up to the top of a file to hit
test
, or even if they want an easy way to test an entire build target.How does a client use this
This is accomplished by introducing a new params object for the
debug-adapter-start
where all a client has to send in is the current file it's in, and therunType
which can berun
,testFile
, ortestTarget
. Env, args, and options are all still possible, but not necessary. Ideally a client will have these three different run types mapped to a command. You can see a very rough example of this in nvim-metals since that's how I was testing this.Some examples
In the case where there is just a single main method (in the build target the file belongs to), it will just be ran. You don't even have to be in that file, just the build target. If there are no mains in your build target, you'll be warned.
In the case of multiple mains in a single build target, you'll be prompted to choose which you'd like to run.
The
testFile
option will check to see if you're currently in a test suite. If so, it runs it, if not you will get a warning about no tests being found in your file. If you use thetestTarget
it will run all the suites in that target. If there are no tests in that target, you'll get a warning.What's left
In this pr I'd still like to get a couple things done, but before I do I want to make sure you're on board with this direction. After that I need to make sure
Closes #2491