This repository has been archived by the owner on Feb 1, 2019. It is now read-only.
(Phase 1) Project wide refactor and generalization #2
Conversation
This file contains 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
ksdme
changed the title
Travis tests have failedHey @ksdme, 3rd Buildsed -i.bak '/bears = GitCommitBear/d' .coafile |
ksdme
commented
May 30, 2018
.coafile
Outdated
| bears = LineLengthBear | ||
| ignore_length_regex = ^.*https?:// | ||
|
|
||
| [commit] | ||
| [all.commit] | ||
| enabled = False |
gaocegege
reviewed
May 31, 2018
| @@ -6,4 +6,4 @@ coverage: | |||
| project: | |||
| default: | |||
| enabled: true | |||
| target: 95% | |||
| target: 100% | |||
There was a problem hiding this comment.
@jayvdb Do we enable the repository in codecov?
There was a problem hiding this comment.
How can this be 100% when the tests are not being run?
There was a problem hiding this comment.
The test is added in another PR
.travis.yml
Outdated
| # - npm test | ||
| # - cd - > /dev/null | ||
| # - coverage run $(which behave) ./tests/server.features | ||
| # - coverage run -a -m unittest discover -s tests |
There was a problem hiding this comment.
Will you implement the test in this PR?
There was a problem hiding this comment.
Yes, I plan to add tests in the next PR.
README.md
Outdated
|
|
||
| [](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. |
There was a problem hiding this comment.
I think A coala language server based on is better.
README.md
Outdated
| ## Known bugs | ||
|
|
||
| * [Language server restarts when `didSave` requests come](https://github.com/coala/coala-vs-code/issues/7) | ||
|
|
||
| ## Reference | ||
|
|
||
| * [python-langserver](https://github.com/sourcegraph/python-langserver) |
requirements.txt
Outdated
| typing>=3.5.3.0 | ||
| coala-bears>=0.10.0.dev20170215041744 | ||
| coala>=0.11.0 | ||
| coala-bears>=0.11.1 | ||
| python-language-server~=0.18.0 |
There was a problem hiding this comment.
Please add some comments about why we use ~= for python-language-server
There was a problem hiding this comment.
We do not. I will replace this dependency with the jsonrpc module that @gatesn forked.
coalals/langserver.py
Outdated
| path = UriUtils.path_from_uri(uri) | ||
|
|
||
| diagnostics = self.local_analyse_file(path) | ||
| logging.basicConfig(level=logging.INFO) |
There was a problem hiding this comment.
Why do we configure the logging here?
coalals/langserver.py
Outdated
| self.root_path = UriUtils.path_from_uri(params['rootPath']) | ||
|
|
||
| return { | ||
| 'capabilities': { |
There was a problem hiding this comment.
Could we avoid hard code here? Maybe we could use a const variable.
coalals/interface.py
Outdated
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class coala_wrapper: |
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?
coalals/interface.py
Outdated
| if workspace is None: | ||
| workspace = '.' | ||
|
|
||
| # change workspace |
There was a problem hiding this comment.
The comment does not have additional information, maybe we could remove it or rewrite it.
coalals/interface.py
Outdated
|
|
||
| @staticmethod | ||
| def _run_coala(): | ||
| stream, retval = StringIO(), -1 |
Travis tests have failedHey @ksdme, 3rd Buildsed -i.bak '/bears = GitCommitBear/d' .coafile |
|
@gaocegege @jayvdb @gatesn Please take a look. |
coalals/interface.py
Outdated
| cycles. | ||
| """ | ||
| result = self._tracked_pool.exec_func( | ||
| coalaWrapper.analyse_file, (file_proxy,), kargs, force=True) |
coalals/concurrency.py
Outdated
| """ | ||
| 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): |
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()?
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.
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.
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.
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
coalals/utils/files.py
Outdated
| return self._contents | ||
|
|
||
| def close(self): | ||
| del self._contents |
There was a problem hiding this comment.
self._contents is a string. This yields a AttributeError.
ksdme
commented
Jun 2, 2018
coalals/utils/entities.py
Outdated
|
|
||
| @classmethod | ||
| def entity_name(cls): | ||
| return cls.__class__.__name__ |
There was a problem hiding this comment.
cls.__class__ is type. This should rather be cls.__name__.
ksdme
commented
Jun 2, 2018
coalals/concurrency.py
Outdated
|
|
||
| def force_free_slots(self): | ||
| if self.has_slots(): | ||
| return |
ksdme
commented
Jun 2, 2018
coalals/concurrency.py
Outdated
| self._job_tracker = JobTracker(max_jobs) | ||
| self._process_pool = ProcessPoolExecutor(max_workers=max_workers) | ||
|
|
||
| def exec_func(self, func, params=[], kargs={}, force=False): |
There was a problem hiding this comment.
Check for types of params and kargs.
ksdme
commented
Jun 2, 2018
coalals/langserver.py
Outdated
| return a Future or a boolean depending on the | ||
| allocation. | ||
| """ | ||
| proxy = self._proxy_map.resolve(filename, self.root_path) |
There was a problem hiding this comment.
coalaWrapper has access to _proxy_map. It should be responsible to resolve the proxy itself.
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
coalals/concurrency.py
Outdated
| 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 |
|
@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 |
|
https://github.com/ksdme/coala-ls/tree/gsoc-phase-1-tests contains the tests for this module. The coverage is at 100% currently. |
|
@sadovnychyi PTAL |
sadovnychyi
reviewed
Jun 5, 2018
|
|
||
| try: | ||
| server = TCPServer((bind_addr, port), wrapper_class) | ||
| except Exception as e: |
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.
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
| @@ -26,12 +26,6 @@ before_install: | |||
| sleep 3; | |||
| fi | |||
|
|
|||
| - cd ./vscode-client | |||
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 😄
There was a problem hiding this comment.
OK, then we could remove it after #7 merged.
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.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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).