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

feat: add path in ConformInfo #244

Merged
merged 1 commit into from
Dec 10, 2023
Merged

Conversation

agoodshort
Copy link
Contributor

Here is my draft for #225

2 questions:

  • Should this be an option or built-in?
  • Are you okay with the type formatter.type being a string? set as nil when initialized?

Any thoughts?

image

@gegoune
Copy link

gegoune commented Dec 9, 2023

Adding extra options for such niche setting doesn't sound right. How about using conceal?

Copy link
Owner

@stevearc stevearc left a comment

Choose a reason for hiding this comment

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

Not sure what you mean with your question about formatter.type
I think it's fine to include the binary path all the time. No need for an option. The info panel should only be used for debugging, so it's fine if it's a little visually busy.

Couple changes requested, but generally on board with the approach

@@ -3,6 +3,7 @@ local M = {}
---@class (exact) conform.FormatterInfo
---@field name string
---@field command string
---@field path? string
Copy link
Owner

Choose a reason for hiding this comment

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

We don't really need to add the path here, do we? We can just run exepath from the info window where we need the full path

Copy link
Contributor Author

@agoodshort agoodshort Dec 10, 2023

Choose a reason for hiding this comment

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

Sorry made a mistake, I meant formatter.path instead of type but here you're answering my question.

Yeah, I think we don't. We only use path in the info panel, so it can be produced there.

table.insert(highlights, {
"DiagnosticInfo",
#lines,
formatter.name:len() + 7 + table.concat(filetypes, ", "):len() + 3,
Copy link
Owner

Choose a reason for hiding this comment

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

Let's pull the table.concat(filetypes, ", ") into a variable so we don't have to run it twice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

@agoodshort
Copy link
Contributor Author

@stevearc I just squashed the requested changes into my commit.

@agoodshort agoodshort marked this pull request as ready for review December 10, 2023 11:57
Copy link
Owner

@stevearc stevearc left a comment

Choose a reason for hiding this comment

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

One nit then LGTM!

table.insert(lines, line)
table.insert(
highlights,
{ "DiagnosticInfo", #lines, formatter.name:len(), formatter.name:len() + 6 }
)
if path ~= nil then
Copy link
Owner

Choose a reason for hiding this comment

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

vim.fn.exepath never returns nil. If the executable is missing, it will return an empty string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right! Thanks for taking the time to review thoroughly

@stevearc
Copy link
Owner

Great, thanks for the PR!

@stevearc stevearc merged commit fb9b050 into stevearc:master Dec 10, 2023
7 checks passed
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.

3 participants