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

add 'full' for line_width in diagnostics options #2452

Merged

Conversation

blue-pitaya
Copy link
Contributor

@blue-pitaya blue-pitaya commented Apr 6, 2023

Description

Added new value "full" for line_width option in builtin.diagnostics. When "full" is set there are no restrictions on diagnostics message line width and so whole message is shown.

image

Until this change, line_width could only be number. Maximum line width was "one width" of picker window and longer messages were cut.

image

This was annoying since you generally want to read whole error message.

Fixes #2020

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list
relevant details about your configuration

  • Verify change
  • in config defaults set wrap_line = true
  • open project where there is error with long message
  • open diagnostics with require('telescope.builtin').diagnostics({line_width='full'})

Configuration:

  • Neovim version (nvim --version): NVIM v0.8.3
  • Operating system and version: Linux 6.2.6-artix1-1

Checklist:

  • My code follows the style guidelines of this project (stylua)
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (lua annotations)

@blue-pitaya
Copy link
Contributor Author

Is this PR good to merge, or should I make some adjustments?

Copy link
Contributor

@jamestrew jamestrew left a comment

Choose a reason for hiding this comment

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

Hey sorry for the late response. Just a couple of suggested changes and I think we're good.

lua/telescope/builtin/init.lua Outdated Show resolved Hide resolved
if line_width == "full" then
line_width_opts = {}
else
error(string.format("'%s' is not a valid value for line_width", line_width))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should move the validation of line_width to declaration of the picker and use utils.notify instead of error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, thanks for checking my PR :)

I changed error to utils.notify, but left it in the same place. I couldn't find "good" place for validation in diagnostics picker, there is no direct call to line_width opt in here. Also, function 'make_entry.gen_from_diagnostics' is used specifically for diagnostics, so I guess it's not the worst place for this validation.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is that unlike with error, utils.notify will let the picker continue to run and it'll eventually throw an actual error that isn't as helpful.

I think we can simplify this bit of code to

  if type(line_width) == "string" and line_width == "full" then
    line_width_opts = {}
  end

then in lua/telescope/builtin/__diagnostics.lua:124, do like

  if type(opts.line_width) == "string" and opts.line_width ~= "full" then
    utils.notify("builtin.diagnostics", {
      msg = string.format("'%s' is not a valid value for line_width", opts.line_width),
      level = "ERROR",
    })
    return
  end

@jamestrew
Copy link
Contributor

Also just as a side note, noticed you're on NVIM v0.8.3 but telescope is only supporting v0.9.0+. You might want to update your neovim if you haven't already lest you might run into various issues.

@blue-pitaya
Copy link
Contributor Author

Hi, sorry for stalling this PR so much! I applied your suggestions.

@jamestrew
Copy link
Contributor

thanks!

@jamestrew jamestrew merged commit dc7f25c into nvim-telescope:master Sep 3, 2023
6 checks passed
rameshsanth pushed a commit to rameshsanth/telescope.nvim that referenced this pull request Nov 17, 2023
* add 'full' for line_width in diagnostics options

* lowercase documentation entry and change error notify method

* moved line_width options checking
@rlpowell
Copy link

See #2020 for example use. :)

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.

Full messages visible in diagnostics picker
3 participants