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

Fix handling notification/request parameters that is not used for class fields name from the server #56

Merged
merged 5 commits into from
Mar 26, 2019

Conversation

tom-tan
Copy link
Contributor

@tom-tan tom-tan commented Mar 22, 2019

Sorry, I accidentally send incomplete request...
I will update it to make this request complete.

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

This request is to handle message fields that is not valid as python class fields (such as __dummy__) from the client.

For example, Eglot, which is a LSP client for emacs send initialized notification with __dummy__ parameter.

Currently language servers that are based on pygls fail with the following exception message:

Process EGLOT (v1/cwl-mode) stderr finished
Traceback (most recent call last):
  File "/Users/tanjo/repos/cwl-language-server/cwl_language_server/main.py", line 42, in <module>
    server.start_io()
  File "/Users/tanjo/.pyenv/versions/3.6.5/lib/python3.6/site-packages/pygls/server.py", line 164, in start_io
    self.lsp.data_received))
  File "/Users/tanjo/.pyenv/versions/3.6.5/lib/python3.6/asyncio/base_events.py", line 468, in run_until_complete
    return future.result()
  File "/Users/tanjo/.pyenv/versions/3.6.5/lib/python3.6/site-packages/pygls/server.py", line 65, in aio_readline
    proxy(body)
  File "/Users/tanjo/.pyenv/versions/3.6.5/lib/python3.6/site-packages/pygls/protocol.py", line 405, in data_received
    object_hook=deserialize_message))
  File "/Users/tanjo/.pyenv/versions/3.6.5/lib/python3.6/json/__init__.py", line 367, in loads
    return cls(**kw).decode(s)
  File "/Users/tanjo/.pyenv/versions/3.6.5/lib/python3.6/json/decoder.py", line 339, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/Users/tanjo/.pyenv/versions/3.6.5/lib/python3.6/json/decoder.py", line 355, in raw_decode
    obj, end = self.scan_once(s, idx)
  File "/Users/tanjo/.pyenv/versions/3.6.5/lib/python3.6/site-packages/pygls/protocol.py", line 86, in deserialize_message
    data.keys())(*data.values())
  File "/Users/tanjo/.pyenv/versions/3.6.5/lib/python3.6/collections/__init__.py", line 409, in namedtuple
    '%r' % name)
ValueError: Field names cannot start with an underscore: '__dummy__'

The reason is that __dummy__ that is sent from Eglot is not valid identifier for the field name in python.
By using rename=True for namedtuple, such fields are replaced with _1, _2 and so on.
Their names are very annoying but is better than server terminations.

Note: cwl_language_server is our project for a language server for Common Workflow Language.
We currently use pylspclient but we start trying pygls as an alternative.

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
    • This request does not add any functions.
  • Standalone docs have been updated accordingly
    • This request does not contain the update that needs the doc update.
  • CONTRIBUTORS.md was updated, as appropriate
  • Changelog has been updated, as needed (see CHANGELOG.md)
    • No notable changes

@tom-tan tom-tan changed the title Fix handling parameters such as dummy from the server Fix handling notification/request parameters that is not used for class fields name from the server Mar 22, 2019
@tom-tan tom-tan changed the title Fix handling notification/request parameters that is not used for class fields name from the server WIP: Fix handling notification/request parameters that is not used for class fields name from the server Mar 22, 2019
@danixeee
Copy link
Contributor

@tom-tan Good catch and thank you for your contribution!

Maybe we should consider adding multiple clients for our json-extension example, because we are testing it just with VS Code.

P.S. Check-boxes in Code review checklist from the PR template should be checked by a code reviewer, so I will uncheck them.

@@ -83,7 +83,7 @@ def deserialize_message(data):
return JsonRPCNotification(**data)

return namedtuple('Object',
Copy link
Contributor

Choose a reason for hiding this comment

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

@tom-tan Can you please remove the line break? Line 85. won't be longer than 79 chars.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@danixeee danixeee self-requested a review March 22, 2019 11:37
@danixeee danixeee added the bug Something isn't working label Mar 22, 2019
@danixeee danixeee added this to Triage in Kanban via automation Mar 22, 2019
@danixeee danixeee moved this from Triage to Review in Kanban Mar 22, 2019
Copy link
Contributor

@danixeee danixeee left a comment

Choose a reason for hiding this comment

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

If you feel comfortable, it would be great to add some tests for deserialize_message to test_protocol.py. No worries if you can't, just let me know, so I can merge the PR otherwise.

@tom-tan
Copy link
Contributor Author

tom-tan commented Mar 25, 2019

Thank you for your suggestion.

I will update the code and add a test for it.

@tom-tan
Copy link
Contributor Author

tom-tan commented Mar 25, 2019

Done!

I added a test for deserialized_message.
I used an actual input for deserializing rather than deserializing '{ "__dummy__": true }' directly.
IMO it is easier to understand what this test wants to check.

@danixeee danixeee self-requested a review March 25, 2019 11:52
@danixeee
Copy link
Contributor

danixeee commented Mar 25, 2019

Looks go to me.

@tom-tan Can you please update the CHANGELOG file? We decided that every notable changes should be added to the CHANGELOG, and I think that this PR's change should be recorded there.

To make it easier for you, here is how it should look:

# Changelog

All notable changes to this project will be documented in this file.

The format is based on [Keep a Changelog][keepachangelog],
and this project adheres to [Semantic Versioning][semver].

## [Unreleased]

### Changed

- Your message here ([#56])

[#56]: https://github.com/openlawlibrary/pygls/pull/56

## [0.7.4] - 03/21/2019

### Added

- Add Pull Request template ([#54])

EDIT: Corrected PR link in example changelog.

@tom-tan tom-tan changed the title WIP: Fix handling notification/request parameters that is not used for class fields name from the server Fix handling notification/request parameters that is not used for class fields name from the server Mar 26, 2019
@tom-tan
Copy link
Contributor Author

tom-tan commented Mar 26, 2019

Done!
I updated CHANGELOG.

Copy link
Contributor

@danixeee danixeee left a comment

Choose a reason for hiding this comment

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

@tom-tan Looks go to me, will merge it soon.

Thank you again for your contribution! Let us know about your experience with pygls and if you have any language server that is based on it, feel free to update Implementations document. :)

@danixeee danixeee merged commit 02cdd57 into openlawlibrary:master Mar 26, 2019
Kanban automation moved this from Review to Done - 2019 Mar 26, 2019
'''
obj = json.loads(params_with_reserved_word,
object_hook=deserialize_message)
assert isinstance(obj, object)
Copy link
Contributor

Choose a reason for hiding this comment

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

In Python, this is True for every valid first argument.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Kanban
  
Done - 2019
Development

Successfully merging this pull request may close these issues.

None yet

2 participants