Update hover.py to use language server argument

[noklam]

Description (e.g. "Related to ...", etc.)

Following the best practice guide from "passing language server instance as parameter"
https://pygls.readthedocs.io/en/latest/pages/user-guide.html#passing-language-server-instance

Code review checklist (for code reviewer to complete)

• Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
• Title summarizes what is changing
• Commit messages are meaningful (see this for details)
• Tests have been included and/or updated, as appropriate
• Docstrings have been included and/or updated, as appropriate
• Standalone docs have been updated accordingly

[alcarney]

Thanks for this!

I see no issue with the code, but the CI lint job is failing due to the formatting of the commit message. Would you be able to edit it to be something like refactor: update hover.py to use language server argument?

[noklam]

Sure. Do you have something like pre-commit to enforce these kind of checking? I didn't know this is a requirement since it's not mentioned in https://github.com/openlawlibrary/pygls/blob/main/CONTRIBUTING.md.

[tombh]

Sorry about that. We hadn't updated that doc in a while. I made a PR to mention the commit message convention: #456

[noklam]

No worries! I've fixed the commit message now and the checks are all passed.

[alcarney]

Great, thanks!

[alcarney]

@tombh would you be open to adding a pre-commit config to repo (and possibly even using the ci version? I'll admit not running the lint job locally always catches me out 😅

[noklam]

I don't mind adding the precommit config if this is wanted

[tombh]

I'm personally not a fan of local commit hooks. I'm a big advocate of linear history, so a lot of quick git commit --amend and in-depth git rebase --interactive. So having automated commands that can potentially fail, especially during an interactive rebase can be problematic. Although of course the hooks can be disabled with --no-verify.

I'm interested in hearing arguments for pre-commits.

What about adding a line in PULL_REQUEST_TEMPLATE.md about running poetry run poe lint before pushing?

[alcarney]

So having automated commands that can potentially fail, especially during an interactive rebase can be problematic.

FWIW, I rebase quite often, and I'm yet to have a major issue caused by a pre-commit hook.
In my experience, because the checks are run before each commit by the time I'm rebasing the commits are already "clean" and are unlikely to fail - though I guess I could just be lucky! 😅

We certainly don't have to go as far as putting pre-commit.ci in place, but what about the pre-commit config file so that there was the option for people to opt into the hooks locally?

What about adding a line in PULL_REQUEST_TEMPLATE.md about running poetry run poe lint before pushing?

Sure, it wouldn't hurt :)

[tombh]

I think the commit message lint has to run after writing the commit message right? Anyway, yes, we can definitely use pre-commit.ci and have it opt-in, I'm totally up for that.

Made a PR for that PULL_REQUEST_TEMPLATE update #460