Migrate pydantic to cattrs

pappasam / jedi-language-server

A Python language server exclusively for Jedi. If Jedi supports it well, this language server should too.

MIT License

574 stars 44 forks source link

Migrate pydantic to cattrs #281

Closed pappasam closed 9 months ago

pappasam commented 9 months ago

Note: tests for completion are now failing in a way that is slightly confusing to me. Will try looking into this later, but am open to suggestions from others (including @PeterJCLaw ) in case the reason is more obvious to others!

PeterJCLaw commented 9 months ago

@pappasam are there any direct tests that the initialisation options are being interpreted correctly?

Related -- what's the expected behaviour if there are additional keys which aren't known to the initialisation options parser?

My testing locally (so far only with test_eager_lsp_completion) suggests that the issue is that the camel casing handling isn't working and that the server is always running with the defaults.

PeterJCLaw commented 9 months ago

Related -- what's the expected behaviour if there are additional keys which aren't known to the initialisation options parser?

Looks like Pydantic and cattrs both silently allow extra unknown fields by default:

https://docs.pydantic.dev/latest/usage/models/#extra-fields
https://catt.rs/en/stable/cattrs.html#cattrs.converters.Converter (see forbid_extra_keys=False)

This seems likely to mask configuration errors to me. Ideally I guess there'd be a way to capture these, log them, but still consume what other config the user passed? (for compatibility with the current behaviour)

PeterJCLaw commented 9 months ago

Ok, I think I have a fix for this. #282 adds testing and I'm ~working on fixing~ have fixed the issues found by those tests in #283.