This repository was archived by the owner on Feb 1, 2019. It is now read-only.
(Phase 1) Project wide refactor and generalization#2
Open
ksdme wants to merge 19 commits into
Open
Conversation
ksdme
changed the title
Travis tests have failedHey @ksdme, 3rd Buildsed -i.bak '/bears = GitCommitBear/d' .coafile |
ksdme
commented
May 30, 2018
|
|
||
| [commit] | ||
| [all.commit] | ||
| enabled = False |
Member
Author
gaocegege
reviewed
May 31, 2018
| default: | ||
| enabled: true | ||
| target: 95% | ||
| target: 100% |
Member
There was a problem hiding this comment.
How can this be 100% when the tests are not being run?
Member
There was a problem hiding this comment.
The test is added in another PR
| # - npm test | ||
| # - cd - > /dev/null | ||
| # - coverage run $(which behave) ./tests/server.features | ||
| # - coverage run -a -m unittest discover -s tests |
Member
There was a problem hiding this comment.
Will you implement the test in this PR?
Member
Author
There was a problem hiding this comment.
Yes, I plan to add tests in the next PR.
|
|
||
| [](https://travis-ci.org/coala/coala-vs-code) | ||
| [](https://codecov.io/gh/coala/coala-vs-code) | ||
| A coala language server [Language Server Protocol (LSP)](https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md). Python versions 3.x is supported. |
Member
There was a problem hiding this comment.
I think A coala language server based on is better.
|
|
||
| ## Reference | ||
|
|
||
| * [python-langserver](https://github.com/sourcegraph/python-langserver) |
| coala-bears>=0.10.0.dev20170215041744 | ||
| coala>=0.11.0 | ||
| coala-bears>=0.11.1 | ||
| python-language-server~=0.18.0 |
Member
There was a problem hiding this comment.
Please add some comments about why we use ~= for python-language-server
Member
Author
There was a problem hiding this comment.
We do not. I will replace this dependency with the jsonrpc module that @gatesn forked.
| path = UriUtils.path_from_uri(uri) | ||
|
|
||
| diagnostics = self.local_analyse_file(path) | ||
| logging.basicConfig(level=logging.INFO) |
Member
There was a problem hiding this comment.
Why do we configure the logging here?
| self.root_path = UriUtils.path_from_uri(params['rootPath']) | ||
|
|
||
| return { | ||
| 'capabilities': { |
Member
There was a problem hiding this comment.
Could we avoid hard code here? Maybe we could use a const variable.
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class coala_wrapper: |
Member
There was a problem hiding this comment.
About the class name, I think we have two patterns: coala_wrapper and LangServer. Could we unify the way?
| if workspace is None: | ||
| workspace = '.' | ||
|
|
||
| # change workspace |
Member
There was a problem hiding this comment.
The comment does not have additional information, maybe we could remove it or rewrite it.
|
|
||
| @staticmethod | ||
| def _run_coala(): | ||
| stream, retval = StringIO(), -1 |
Travis tests have failedHey @ksdme, 3rd Buildsed -i.bak '/bears = GitCommitBear/d' .coafile |
Member
Author
|
@gaocegege @jayvdb @gatesn Please take a look. |
gaocegege
reviewed
Jun 1, 2018
| @@ -0,0 +1,8 @@ | |||
| class LSP_Entity: | |||
| cycles. | ||
| """ | ||
| result = self._tracked_pool.exec_func( | ||
| coalaWrapper.analyse_file, (file_proxy,), kargs, force=True) |
| """ | ||
| JobTracker helps to keep track of running jobs and | ||
| function as an advanced counter. It is lazy and will | ||
| only updated its internal state when requested for |
| JobTracker and func_wrapper. | ||
| """ | ||
|
|
||
| def __init__(self, max_jobs=1, max_workers=1): |
Member
There was a problem hiding this comment.
Any particular reason to default max_workers=1 across the files? Why not just use the user's multiprocessing.cpu_count()?
Member
Author
There was a problem hiding this comment.
It should generally suffice having max_jobs == max_workers with a soft upper cap of cpu_count() on max_workers. Since the default for max_jobs is 1, max_workers can safely be 1.
Member
There was a problem hiding this comment.
Hmm, maybe I misunderstand something, but isn't max_jobs eventually going to be a user preference? In which case, having max_workers=multiprocessing.cpu_count() makes sense(since that should probably never be exposed to user preferences)?
It's fine if you want to make those changes later(when it does indeed become a preference), I just thought it's a more sensible default.
Member
Author
There was a problem hiding this comment.
I agree with you suggestion. I think max_jobs will be a user preference sometime later. I'll defer this change until the time it is actually exposed.
Member
Author
There was a problem hiding this comment.
Also please review the parts of code I just added.
Travis tests have failedHey @ksdme, 3rd Buildsed -i.bak '/bears = GitCommitBear/d' .coafile |
ksdme
commented
Jun 1, 2018
| return self._contents | ||
|
|
||
| def close(self): | ||
| del self._contents |
Member
Author
There was a problem hiding this comment.
self._contents is a string. This yields a AttributeError.
ksdme
commented
Jun 2, 2018
|
|
||
| @classmethod | ||
| def entity_name(cls): | ||
| return cls.__class__.__name__ |
Member
Author
There was a problem hiding this comment.
cls.__class__ is type. This should rather be cls.__name__.
ksdme
commented
Jun 2, 2018
|
|
||
| def force_free_slots(self): | ||
| if self.has_slots(): | ||
| return |
ksdme
commented
Jun 2, 2018
| self._job_tracker = JobTracker(max_jobs) | ||
| self._process_pool = ProcessPoolExecutor(max_workers=max_workers) | ||
|
|
||
| def exec_func(self, func, params=[], kargs={}, force=False): |
Member
Author
There was a problem hiding this comment.
Check for types of params and kargs.
ksdme
commented
Jun 2, 2018
| return a Future or a boolean depending on the | ||
| allocation. | ||
| """ | ||
| proxy = self._proxy_map.resolve(filename, self.root_path) |
Member
Author
There was a problem hiding this comment.
coalaWrapper has access to _proxy_map. It should be responsible to resolve the proxy itself.
Member
Author
There was a problem hiding this comment.
It should handle a proxy resolution failing because of failed attempt to open file.
ksdme
commented
Jun 2, 2018
| return not job.done() | ||
|
|
||
| def __init__(self, max_jobs=1): | ||
| self._max_jobs = max_jobs |
Travis tests have failedHey @ksdme, 3rd Buildsed -i.bak '/bears = GitCommitBear/d' .coafile |
Member
|
@jayvdb PTAL |
Travis tests have failedHey @ksdme, 3rd Buildsed -i.bak '/bears = GitCommitBear/d' .coafile |
Travis tests have failedHey @ksdme, 3rd Buildsed -i.bak '/bears = GitCommitBear/d' .coafile |
Member
Author
|
https://github.com/ksdme/coala-ls/tree/gsoc-phase-1-tests contains the tests for this module. The coverage is at 100% currently. |
Member
Author
|
@sadovnychyi PTAL |
sadovnychyi
reviewed
Jun 5, 2018
|
|
||
| try: | ||
| server = TCPServer((bind_addr, port), wrapper_class) | ||
| except Exception as e: |
Member
There was a problem hiding this comment.
Since you are going to run this from CLI I think you should catch a KeyboardInterrupt separately and just exit('Command killed by keyboard interrupt') or something like that.
Member
Author
There was a problem hiding this comment.
Yes, the TCP mode will mostly be used from CLI during debugging.
Travis tests have failedHey @ksdme, 3rd Buildsed -i.bak '/bears = GitCommitBear/d' .coafile |
gaocegege
approved these changes
Jun 10, 2018
| sleep 3; | ||
| fi | ||
|
|
||
| - cd ./vscode-client |
Member
There was a problem hiding this comment.
Please do not import the changes about vscode in this PR since we have a separate PR to do it 😄
Member
Author
This was referenced Jun 10, 2018
Open
ksdme
force-pushed
the
gsoc-phase-1
branch
2 times, most recently
from
6601489 to
c447b25
Compare
Travis tests have failedHey @ksdme, 1st Build
|
Update .gitignore with .idea/ as per Gradle and Maven auto import.
Drop vscode client since no component of coala-ls now is specific to or depends on VSCode.
This updates the README text and updates the demo assets.
Due to the heavy restructuring required to adapt this module into architecture for coala-ls the old codebase is dropped.
Add test resource file with intentional syntax breaks or containing expected diagnostics. Add test helpers.
Configures test env to add coalals to sys.path.
Adds files modules and corrosponding tests.
Add log.py that can handle configuring logging.
Add wrappers for methods and streams.
Add support for job tracking, concurrent execution via abstract process pools.
Add initial mechanism to run coala analyser and gather the results.
Add module to process the results and convert coala diagnostic messages to language server supported messages. Supports code fixing messages also.
Add language server class capable of handling event notifications and respond accordingly.
Add functions to invoke language server instance with respective streams as per the invocation mode.
Add __main__ to make coalals an executable module.
Update the requirement to latest versions.
Update the test requirements to use pytest.
- Increase coverage requirement to 100%. - Update omissions in coverage. - Updates travis to use py.test.
Add script to start coala-ls.
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This rebases and updates the project with some fixes, improves the code style and adds generalization (Concurrent coala analysis cycles, result processing, logging, file proxies).