API: drop None rather than unset in JSON response #236

tombh · 2022-05-31T13:34:05Z

This addresses #217 and #231.

I am still new to the project, so I may not have the full story. But it
started with a somewhat far-reaching merge to remove defaults from all
serialisable fields in the API: #198. This had the knock-on effect of
regressing progress notifications, as reported by @MatejKastak in #217.
One of their suggestions is to not use exclude_unset=True. Some months
later @dimbleby suggested that exclude_none=True would also fix the
issue[1]. Considering those 2 suggestions, let's replace
exclude_unset=True with exclude_none=True.

remove defaults from optional fields #198 (comment)

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)

tombh · 2022-05-31T13:42:42Z

@MatejKastak and @dimbleby: would you like to comment on this fix?

dimbleby · 2022-05-31T13:46:50Z

Looks familiar! sure, seems ok to me

MatejKastak · 2022-05-31T13:54:17Z

Great! Thanks for working on this. I also approve and I don't have any other comments.

tombh · 2022-05-31T14:12:54Z

@dimbleby haha, yes you're fix 😏 Thank you

@MatejKastak Thank you too, and thanks for waiting

Just waiting for the review, then LGTM 🚀

renatav

LGTM, just one question

renatav · 2022-05-31T22:44:01Z

tests/lsp/test_progress.py

+            CODE_LENS,
+            CodeLensOptions(
+                resolve_provider=False,
+                work_done_progress='token'


Any reason why PROGRESS_TOKEN is not used here?

perrinjerome · 2022-06-01T00:14:47Z

Can you also add something like:

Fixes #231

in the description so that this issue gets automatically fixed ?

perrinjerome

Good to see this :)

perrinjerome · 2022-06-01T00:15:49Z

CONTRIBUTORS.md

@@ -15,3 +15,4 @@
 - [yorodm](https://github.com/yorodm)
 - [bollwyvl](https://github.com/bollwyvl)
 - [Dillan Mills](https://github.com/DillanCMills)
+- [Tom BH](https://github.com/tombh)


It seems it was not always followed earlier either, but this list is supposed to be sorted in alphabetical order ( https://github.com/openlawlibrary/pygls/blob/f53d6615e4561184b393d4b2c6b5629f31f33dbb/CONTRIBUTORS.md#contributors-alphabetical )

perrinjerome · 2022-06-01T00:36:21Z

Can you also add something like:

Fixes #231

in the description so that this issue gets automatically fixed ?

Oh sorry, I did not realize the issue was already mentioned in the pull request description 😖.

That said, you can still change to using a keyword like Fixes if you wish, because it is understood by github ( https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword )

renatav · 2022-06-01T12:26:48Z

@tombh I don't think that this issue is specific to this PR, but Python 3.6 Windows build is getting stuck. Would be good to know what is causing this even though we are planning to drop Python 3.6 support

@MatejKastak

Fixes #217, #231. I am still new to the project, so I may not have the full story. But it started with a somewhat far-reaching merge to remove defaults from all serialisable fields in the API: #198. This had the knock-on effect of regressing progress notifications, as reported by @MatejKastak in #217. One of their suggestions is to not use `exclude_unset=True`. Some months later @dimbleby suggested that `exclude_none=True` would also fix the issue[1]. Considering those 2 suggestions, let's replace `exclude_unset=True` with `exclude_none=True`. 1. #198 (comment)

tombh · 2022-06-01T21:45:03Z

@perrinjerome I added the "Fixes" to the commit message and changed the contributors order. Thanks

@renatav Yes I saw that, and have had a play with it, it's actually intermittent and I've even seen it affect almost all items in the matrix, any version or OS, but yes more often Windows/Python 3.6. There's evidence of it happening before this PR, so as you say I don't think it's being introduced with this code. There's no lock file for the dependencies, so the only thing I can think is that a minor change in a dependent package has caused this. Which leads me to think that our next step should be #229, #223 and of course #235. So I vote for merging this PR as all the tests are passing, just rarely without some failure from the flakey bug.

renatav · 2022-06-01T22:08:29Z

@tombh We usually use fixes in the PR comment, not sure that commit message with fixes will automatically close issues. I'll approve the PR

This undoes the change in openlawlibrary#236 as well as bring back the explicit setting of non `None` default fields as suggested in [1] This commit also introduces serialization test cases that hopefully cover all the scenarios raised in openlawlibrary#245, openlawlibrary#231 and openlawlibrary#198 [1]: openlawlibrary#245 (comment)

This undoes the change in #236 as well as bring back the explicit setting of non `None` default fields as suggested in [1] This commit also introduces serialization test cases that hopefully cover all the scenarios raised in #245, #231 and #198 [1]: #245 (comment)

tombh force-pushed the exclude-nones-rather-than-unsets branch from 88c4c2e to fb73307 Compare May 31, 2022 13:37

tombh requested a review from renatav May 31, 2022 13:38

tombh force-pushed the exclude-nones-rather-than-unsets branch from fb73307 to f53d661 Compare May 31, 2022 13:39

renatav reviewed May 31, 2022

View reviewed changes

perrinjerome previously approved these changes Jun 1, 2022

View reviewed changes

tombh dismissed perrinjerome’s stale review via e3ee7ee June 1, 2022 19:52

tombh force-pushed the exclude-nones-rather-than-unsets branch 3 times, most recently from b1e937a to ddde14f Compare June 1, 2022 21:05

tombh force-pushed the exclude-nones-rather-than-unsets branch from ddde14f to 45879b4 Compare June 1, 2022 21:18

renatav self-requested a review June 1, 2022 22:08

renatav approved these changes Jun 1, 2022

View reviewed changes

tombh closed this Jun 2, 2022

tombh reopened this Jun 2, 2022

tombh merged commit 0d2b7cd into master Jun 2, 2022

tombh mentioned this pull request Jun 2, 2022

Progress not functional because kind is missing #231

Closed

alcarney mentioned this pull request Jul 3, 2022

exclude_none = True breaks shutdown requests #245

Closed

alcarney mentioned this pull request Jul 4, 2022

Change exclude_none back to exclude_unset #247

Merged

8 tasks

tombh deleted the exclude-nones-rather-than-unsets branch July 5, 2022 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: drop None rather than unset in JSON response #236

API: drop None rather than unset in JSON response #236

tombh commented May 31, 2022 •

edited

Loading

tombh commented May 31, 2022

dimbleby commented May 31, 2022

MatejKastak commented May 31, 2022

tombh commented May 31, 2022

renatav left a comment

renatav May 31, 2022

tombh Jun 1, 2022

perrinjerome commented Jun 1, 2022

perrinjerome left a comment

perrinjerome Jun 1, 2022

perrinjerome commented Jun 1, 2022 •

edited

Loading

renatav commented Jun 1, 2022

tombh commented Jun 1, 2022

renatav commented Jun 1, 2022

API: drop None rather than unset in JSON response #236

API: drop None rather than unset in JSON response #236

Conversation

tombh commented May 31, 2022 • edited Loading

Code review checklist (for code reviewer to complete)

tombh commented May 31, 2022

dimbleby commented May 31, 2022

MatejKastak commented May 31, 2022

tombh commented May 31, 2022

renatav left a comment

Choose a reason for hiding this comment

renatav May 31, 2022

Choose a reason for hiding this comment

tombh Jun 1, 2022

Choose a reason for hiding this comment

perrinjerome commented Jun 1, 2022

perrinjerome left a comment

Choose a reason for hiding this comment

perrinjerome Jun 1, 2022

Choose a reason for hiding this comment

perrinjerome commented Jun 1, 2022 • edited Loading

renatav commented Jun 1, 2022

tombh commented Jun 1, 2022

renatav commented Jun 1, 2022

tombh commented May 31, 2022 •

edited

Loading

perrinjerome commented Jun 1, 2022 •

edited

Loading