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

Use default settings if initialization options is empty or not provided #11566

Merged
merged 2 commits into from
May 27, 2024

Conversation

dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented May 27, 2024

Summary

This PR fixes the bug to avoid flattening the global-only settings for the new server.

This was added in #11497, possibly to correctly de-serialize an empty value ({}). But, this lead to a bug where the configuration under the settings key was not being read for global-only variant.

By using #[serde(default)], we ensure that the settings field in the GlobalOnly variant is optional and that an empty JSON object {} is correctly deserialized into GlobalOnly with a default ClientSettings instance.

fixes: #11507

Test Plan

Update the snapshot and existing test case. Also, verify the following settings in Neovim:

  1. Nothing
ruff = {
  cmd = {
    '/Users/dhruv/work/astral/ruff/target/debug/ruff',
    'server',
    '--preview',
  },
}
  1. Empty dictionary
ruff = {
  cmd = {
    '/Users/dhruv/work/astral/ruff/target/debug/ruff',
    'server',
    '--preview',
  },
  init_options = vim.empty_dict(),
}
  1. Empty settings
ruff = {
  cmd = {
    '/Users/dhruv/work/astral/ruff/target/debug/ruff',
    'server',
    '--preview',
  },
  init_options = {
    settings = vim.empty_dict(),
  },
}
  1. With some configuration:
ruff = {
  cmd = {
    '/Users/dhruv/work/astral/ruff/target/debug/ruff',
    'server',
    '--preview',
  },
  init_options = {
    settings = {
      configuration = '/tmp/ruff-repro/pyproject.toml',
    },
  },
}

@dhruvmanila

This comment was marked as resolved.

@dhruvmanila dhruvmanila added bug Something isn't working server Related to the LSP server labels May 27, 2024
Copy link

codspeed-hq bot commented May 27, 2024

CodSpeed Performance Report

Merging #11566 will improve performances by 11.03%

Comparing dhruv/avoid-flatten (0b9fd71) with main (6be00d5)

Summary

⚡ 1 improvements
✅ 29 untouched benchmarks

Benchmarks breakdown

Benchmark main dhruv/avoid-flatten Change
parser[numpy/ctypeslib.py] 1.2 ms 1 ms +11.03%

@dhruvmanila dhruvmanila marked this pull request as ready for review May 27, 2024 12:24
@dhruvmanila dhruvmanila changed the title Avoid flattening global only server settings Use default settings if initialization options is empty or not provided May 27, 2024
Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

If we have none. Should we add a test for an empty settings object?

@dhruvmanila
Copy link
Member Author

There is a test :)

#[test]
fn test_empty_init_options_deserialize() {
let options: InitializationOptions = deserialize_fixture(EMPTY_INIT_OPTIONS_FIXTURE);
assert_eq!(options, InitializationOptions::default());
}

@dhruvmanila dhruvmanila merged commit 37ad994 into main May 27, 2024
19 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/avoid-flatten branch May 27, 2024 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working server Related to the LSP server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ruff server in v0.4.5 doesn't consider the configuration setting in Neovim
2 participants