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

Update hover.py to use language server argument #455

Merged
merged 1 commit into from
May 9, 2024

Conversation

noklam
Copy link
Contributor

@noklam noklam commented May 7, 2024

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

@tombh tombh requested a review from alcarney May 7, 2024 15:48
@alcarney
Copy link
Collaborator

alcarney commented May 7, 2024

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
Copy link
Contributor Author

noklam commented May 9, 2024

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
Copy link
Collaborator

tombh commented May 9, 2024

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

@noklam
Copy link
Contributor Author

noklam commented May 9, 2024

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

Copy link
Collaborator

@alcarney alcarney left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@alcarney alcarney merged commit 9660f3f into openlawlibrary:main May 9, 2024
16 checks passed
@alcarney
Copy link
Collaborator

alcarney commented May 9, 2024

@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
Copy link
Contributor Author

noklam commented May 9, 2024

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

@tombh
Copy link
Collaborator

tombh commented May 16, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

tombh commented May 19, 2024

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

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