Skip to content

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

---@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!

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
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