Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

RunTest with CodeLens #937

Merged
merged 7 commits into from
Apr 23, 2017
Merged

Conversation

goenning
Copy link
Contributor

Same feature as Test at Cursor, but in this case there is no need to select anything.

It's based on C# VSCode extension.

What you think about it?

I didn't provide any flag to disable it, because it's possible to do so with standard editor flag: "editor.codeLens": false.

npz78pjlim

@msftclas
Copy link

@goenning,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the idea. Left a suggestion on some refactoring ideas.

return testFunctions.map(func => {
let showReferences: Command = {
title: 'run test',
command: 'go.test.function',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of creating a new command, I'd prefer re-using the existing go.test.cursor.
You can pass the arguments as { functionName: func.name}. And update testAtCursorto look forargs.functionName`. If it exists use it, else use the existing logic to find the test function under the cursor

import { getTestFunctions } from './goTest';

export class GoRunTestCodeLensProvider implements CodeLensProvider {
public provideCodeLenses(document: TextDocument, token: CancellationToken): CodeLens[] | Thenable<CodeLens[]> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding a codelens at the package level as well? Every go file has a package statement up top. So we can add a code lens there and map it to the go.test.package command

@goenning
Copy link
Contributor Author

goenning commented Apr 21, 2017

Done. Also included run file tests.

screen shot 2017-04-21 at 07 33 28

const range = pkg.location.range;
return [
new CodeLens(range, {
title: 'run package tests',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 cents suggestion: s/run/Run. The same for run file tests, run test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about Run tests in package, Run tests in file and Run test ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally prefer lowercase + shorter text to not get much attention. It should be as unobtrusive as possible so devs can focus on code an not on these actions. It's like this on C# codelens

Next enhancement could be debug test 😄

Copy link
Contributor

@ramya-rao-a ramya-rao-a Apr 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as unobtrusive as possible

well, when you put it that way it makes sense.

Next enhancement could be debug test

There is already an ask for it #879 :)

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a comment on the text of the codlens, everything else looks good. Thanks!

@ramya-rao-a ramya-rao-a merged commit 1302fcf into microsoft:master Apr 23, 2017
@ramya-rao-a
Copy link
Contributor

This feature is now out in the latest update (0.6.60)
Thanks @goenning !

@ironcladlou
Copy link
Contributor

Cool feature! Any objection to a feature level config option? I can imagine wanting to disable this feature without disabling CodeLens generally.

@ramya-rao-a
Copy link
Contributor

For references codelens, we added the setting, because guru is known to be slow on Windows and people might want to turn it off, due to multiple guru spinning for long times.

For the run and debug tests, there is no downside perf wise and it doesnt hinder any workflow so didnt add the setting. (Also our settings are really getting big)

Any particular reason you would want to turn it off?

@ironcladlou
Copy link
Contributor

@ramya-rao-a

Any particular reason you would want to turn it off?

Perhaps the user only uses keyboard shortcuts for this and doesn't want an editor row occupied by the hints for every function. Vertical editor space is often precious.

@goenning
Copy link
Contributor Author

goenning commented May 4, 2017

I guess it's up for discussion, but I share same feelings about the amount of settings the extension already has.

You can disable it on workspace level, which should be roughly the same as a feature level setting. The only downside I can see is if your workspace has code from multiple languages and you want to disable codelens just for Go.

@goenning goenning deleted the codelens-runtest branch May 4, 2017 21:08
@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented May 4, 2017

Vertical editor space is often precious.

editor.codeLens can be set to false for no codelens at all.

Lets wait for some more feedback. If there is more ask for setting on each type of codelens, then we can introduce it later.

@ramya-rao-a
Copy link
Contributor

@ironcladlou See #964 (comment)

@coder543
Copy link

Personally, I think this feature is awesome. I hope we can have more codelens integration!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants