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

perrinjerome · 2022-04-03T03:42:31Z

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)

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
CONTRIBUTORS.md was updated, as appropriate
Changelog has been updated, as needed (see CHANGELOG.md)

alcarney

Looks fine to me 😄

alcarney · 2022-04-20T19:29:34Z

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

perrinjerome · 2022-06-24T00:21:56Z

/cc @tombh

tombh · 2022-07-04T13:05:42Z

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 · 2022-07-04T14:32:26Z

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/** :

pygls/.github/workflows/ci.yml

Lines 7 to 9 in 96bc69d

    
           pull_request: 
        
             paths-ignore: 
        
               - "examples/json-extension/**"

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.saobby.my.eu.orgmunity/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 · 2022-07-04T14:45:37Z

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 · 2022-07-05T06:05:37Z

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

This was also my idea.

There are also paths here:

pygls/.github/workflows/json-extension.yml

Lines 5 to 7 in 96bc69d

    
           paths: 
        
             - ".github/workflows/json-extension.yml" 
        
             - "examples/json-extension/**"

pygls/.github/workflows/json-extension.yml

Lines 9 to 11 in 96bc69d

    
           paths: 
        
             - ".github/workflows/json-extension.yml" 
        
             - "examples/json-extension/**"

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 · 2022-07-05T20:41:33Z

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

perrinjerome · 2022-07-07T13:46:31Z

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

By ignoring CI workflow when json extension is changed, we have a problem that the tests are not run and stay in *Expected — Waiting for status to be reported* state as we could see in openlawlibrary#230 Json extension tests were not run when only pygls code is changed, but it can also be interesting to run the test for the extension every time, because it acts as an integration test for pygls. This change the workflow to run all tests for every pull request or push, regardless of what code is changed.

tombh · 2022-07-21T12:54:02Z

Sorry, were you waiting on a review for this?

perrinjerome · 2022-07-25T09:35:12Z

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 #250 to be merged and then update this branch to get test passing.

By ignoring CI workflow when json extension is changed, we have a problem that the tests are not run and stay in *Expected — Waiting for status to be reported* state as we could see in #230 Json extension tests were not run when only pygls code is changed, but it can also be interesting to run the test for the extension every time, because it acts as an integration test for pygls. This change the workflow to run all tests for every pull request or push, regardless of what code is changed.

tombh · 2022-07-25T19:56:39Z

Ah yes, I remember. Ok merged #250.

perrinjerome · 2022-12-04T14:26:05Z

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

tombh · 2022-12-05T21:25:34Z

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.

The spec [1] and pygls implementation [2] needs 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. 1: https://microsoft.github.io/language-server-protocol/specifications/specification-current/#serverInitiatedProgress 2: https://github.com/openlawlibrary/pygls//blob/ee17487e4f40f971e7ec6f7711fa8334c5b7b127/pygls/progress.py#L28

perrinjerome · 2022-12-14T03:05:54Z

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

tombh · 2022-12-14T13:20:45Z

Great, thanks ✨

tombh · 2023-02-16T12:15:26Z

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

alcarney previously approved these changes Apr 20, 2022

View reviewed changes

perrinjerome force-pushed the fix/json-example-progress-token branch from a5e7111 to cf887ad Compare June 24, 2022 00:19

perrinjerome dismissed alcarney’s stale review via a39bb9e July 4, 2022 14:15

perrinjerome force-pushed the fix/json-example-progress-token branch from cf887ad to a39bb9e Compare July 4, 2022 14:15

perrinjerome mentioned this pull request Jul 8, 2022

.github/workflows: always run workflows #250

Merged

4 tasks

tombh previously approved these changes Jul 21, 2022

View reviewed changes

alcarney mentioned this pull request Nov 18, 2022

v1.0.0 BREAKING CHANGES release thread #273

Merged

5 tasks

tombh dismissed their stale review via 463a120 December 5, 2022 21:17

tombh force-pushed the fix/json-example-progress-token branch from f29ff00 to 463a120 Compare December 5, 2022 21:17

perrinjerome force-pushed the fix/json-example-progress-token branch from 463a120 to 2d2e776 Compare December 14, 2022 02:47

Changelog entry

486c871

perrinjerome requested a review from tombh December 14, 2022 03:05

tombh approved these changes Dec 14, 2022

View reviewed changes

tombh merged commit af3f318 into openlawlibrary:master Dec 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

perrinjerome commented Apr 3, 2022 •

edited

Loading

alcarney left a comment

alcarney commented Apr 20, 2022

perrinjerome commented Jun 24, 2022

tombh commented Jul 4, 2022

perrinjerome commented Jul 4, 2022

tombh commented Jul 4, 2022

perrinjerome commented Jul 5, 2022

tombh commented Jul 5, 2022

perrinjerome commented Jul 7, 2022

tombh commented Jul 21, 2022

perrinjerome commented Jul 25, 2022

tombh commented Jul 25, 2022

perrinjerome commented Dec 4, 2022

tombh commented Dec 5, 2022

perrinjerome commented Dec 14, 2022

tombh commented Dec 14, 2022

tombh commented Feb 16, 2023

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

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

Conversation

perrinjerome commented Apr 3, 2022 • edited Loading

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

Code review checklist (for code reviewer to complete)

alcarney left a comment

Choose a reason for hiding this comment

alcarney commented Apr 20, 2022

perrinjerome commented Jun 24, 2022

tombh commented Jul 4, 2022

perrinjerome commented Jul 4, 2022

tombh commented Jul 4, 2022

perrinjerome commented Jul 5, 2022

tombh commented Jul 5, 2022

perrinjerome commented Jul 7, 2022

tombh commented Jul 21, 2022

perrinjerome commented Jul 25, 2022

tombh commented Jul 25, 2022

perrinjerome commented Dec 4, 2022

tombh commented Dec 5, 2022

perrinjerome commented Dec 14, 2022

tombh commented Dec 14, 2022

tombh commented Feb 16, 2023

perrinjerome commented Apr 3, 2022 •

edited

Loading