docs: fix typo in `server.py`

openlawlibrary / pygls

A pythonic generic language server

https://pygls.readthedocs.io/en/latest/

Apache License 2.0

546 stars 101 forks source link

docs: fix typo in `server.py` #471

Closed Viicos closed 1 month ago

Viicos commented 1 month ago

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

Please replace this description with a concise description of this Pull Request.

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

Automated linters

You can run the lints that are run on CI locally with:

poetry install --all-extras --with dev
poetry run poe lint

alcarney commented 1 month ago

@tombh do you think we could remove the Required constraint on the pipelines?

We wouldn't merge with failing check(s) anyway and it seems to be preventing simple PRs like this from being merged...

tombh commented 1 month ago

Are you talking about the "Approve To Run" button for PRs from public forks? https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

The idea is that it prevents potential credential stealing. If anybody can trigger a CI run, then they could also submit code that collected all the private ENV variables and secrets used in CI. So approving each CI run is just an extra security layer.

I think this PR just needs to updated from main?

alcarney commented 1 month ago

Are you talking about the "Approve To Run" button for PRs from public forks?

No, after that, once we've clicked "Approve" and GitHub starts spinning up all of the workflows.

This screenshot is from #465, since there are no code changes most of the workflows are (quite rightly) skipped. However, since they are marked as Required criteria for the PR to be merged the merge button will not be enabled.

So, I'm wondering if it is worth us removing the Required flag from each of the workflows? I trust us to not merge when there is a failing check :)

tombh commented 1 month ago

I'm pretty sure #465 is not running CI because the branch is out of date with main branch. It's because of this repo setting:

The idea being that if PRs were to only receive updates from main at the moment of merge (this is theoretically possible when Git can do a so-called "fast-forward" merge) then the CI hasn't actually truly tested whether the PR doesn't introduce regressions.

alcarney commented 4 weeks ago

Ah ok, I think I'd assumed CI would run even if the branch was behind.