Skip to content
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

Improvement: adds details to github issue #4966

Merged
merged 1 commit into from
Feb 14, 2023

Conversation

@kasiaMarek kasiaMarek force-pushed the github-issues-add-info branch from 27b638c to 6e5d120 Compare February 13, 2023 10:46
@kasiaMarek kasiaMarek marked this pull request as ready for review February 13, 2023 10:57
@kasiaMarek kasiaMarek requested a review from tgodzik February 13, 2023 10:58
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Great work! I have some small suggestions, but this would be a really great improvement.

We should probably suggest using this command as a primary mode of reproting information. (We can point to it in the issue template after the release)

@kasiaMarek kasiaMarek force-pushed the github-issues-add-info branch from 6e5d120 to c066cc7 Compare February 13, 2023 17:03
@kasiaMarek kasiaMarek requested a review from tgodzik February 13, 2023 17:04
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Great work, added some minor comments.

"Open the Metals repository on GitHub to ask a question or report a bug.",
)

val OpenFeatureRequest = new OpenBrowserCommand(
Copy link
Contributor

Choose a reason for hiding this comment

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

You might need to add it to the list of commands at the bottom of the file here, since they need to be registered at the start for the client to pick them up

Copy link
Contributor Author

@kasiaMarek kasiaMarek Feb 14, 2023

Choose a reason for hiding this comment

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

Now, those are only visible in the tree view. But we do want to make it possible to add it to the command pallet. Is that what you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ach, actually my bad. This is done via EchoCommand, but maybe it's good to let it work in Metals comamnds.

currentBuildServer()
.map(s => s"${s.main.name} v${s.main.version}")
.orElse {
calculateNewBuildServer() match {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this is not an actual data point here, since if you need to calcualte it then it's not connected. Maybe you could make the name soemthing like Disconnected: Bloop etc. ?

@kasiaMarek kasiaMarek force-pushed the github-issues-add-info branch from c066cc7 to 703bd46 Compare February 14, 2023 09:04
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

LGTM!

@tgodzik
Copy link
Contributor

tgodzik commented Feb 14, 2023

You need to run sbt scalafixAll before merging

@kasiaMarek kasiaMarek force-pushed the github-issues-add-info branch from 703bd46 to 1f6654b Compare February 14, 2023 10:16
@tgodzik tgodzik merged commit aa943d5 into scalameta:main Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants