search

openlawlibrary / pygls

A pythonic generic language server
https://pygls.readthedocs.io/en/latest/
Apache License 2.0
588 stars 105 forks source link

json-extension: use an unique token for progress example #230

Closed perrinjerome closed 1 year ago

perrinjerome commented 2 years ago

The spec 1 and pygls implementation 2 need the token to be unique.

By using an hardcoded token like this, the progress works only one time, the second time there's an error that token is already registered.

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)

alcarney commented 2 years ago

@dgreisen not sure what is up with these expected statuses...

perrinjerome commented 2 years ago

/cc @tombh

tombh commented 2 years ago

I can't see what's going on with these expected CI status either 🫤 Maybe rebase master, CI config has changed a bit since the PR was submitted. Otherwise this is an easy LGTM

perrinjerome commented 2 years ago

it was already rebased on master, but I thought "let's try again :)" so I changed the commit and git push --forceed again, but same.

I noticed that the "ci" workflow config does not run because it is configured to skip this ci workflow for paths in examples/json-extension/** : https://github.com/openlawlibrary/pygls/blob/96bc69df9c963979ce76f386e83fa059093bc284/.github/workflows/ci.yml#L7-L9

this pull request only change these paths, so this explains why the workflows did not run.

Next question is why this prevents the merge ? Maybe this is because the branch must be marked as "protected" with a setting to only allow merging if these test pass ( https://github.community/t/github-actions-pull-requests-always-include-some-outdated-checks/16157/2 seems a similar issue ). Do you have access to these settings, can you confirm that ?

Thanks !

tombh commented 2 years ago

Well-spotted. I don't have access to those settings, as I discovered from similar issues in #223. We can ask @dgreisen if needs be. But my feeling is that the disadvantages of that paths-ignore setting outweigh its advantages. Its advantage is that it reduces a bit of unnecessary CI work, but CI is essentially free anyway, so it's not really an advantage. And the disadvantage is the situation we have here.

So I vote for just removing those ignore settings. What do you think?

perrinjerome commented 2 years ago

So I vote for just removing those ignore settings. What do you think?

This was also my idea.

There are also paths here:

https://github.com/openlawlibrary/pygls/blob/96bc69df9c963979ce76f386e83fa059093bc284/.github/workflows/json-extension.yml#L5-L7

https://github.com/openlawlibrary/pygls/blob/96bc69df9c963979ce76f386e83fa059093bc284/.github/workflows/json-extension.yml#L9-L11

which if I understand correctly mean "don't run json extension test unless json extension is changed", but I feel it's better to run the tests anyway in that case, a change in pygls might introduce a regression that would only be revealed by the extension test.

tombh commented 2 years ago

Yes, agreed. Do you want to push a commit with those lines removed then?

perrinjerome commented 2 years ago

Yes, agreed. Do you want to push a commit with those lines removed then?

Thanks @tombh I'll try to send a pull request with that, then we'll rebase this one on top

tombh commented 2 years ago

Sorry, were you waiting on a review for this?

perrinjerome commented 2 years ago

Sorry, were you waiting on a review for this?

Thanks @tombh this is not really waiting for review, this is still pending because there's a problem with test status. My idea was to wait for https://github.com/openlawlibrary/pygls/pull/250 to be merged and then update this branch to get test passing.

tombh commented 2 years ago

Ah yes, I remember. Ok merged #250.

perrinjerome commented 1 year ago

I guess this can be merged, we were waiting to have the test passing first, but tests are OK now it seems

tombh commented 1 year ago

I was just about to merge this when I realised that this would be a good addition the changelog. Could you add it?

I rebased master in, which is a force push. Which isn't good practice when collaborating on a branch, my apologies. I just thought I was on the point of merging. You can just force push yourself anyway, because all it is is the recent master branch changes.

perrinjerome commented 1 year ago

Thanks @tombh and sorry for delay, I have finally added a changelog entry

tombh commented 1 year ago

Great, thanks ✨

tombh commented 1 year ago

Released in https://pypi.org/project/pygls/1.0.1/