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

Resolve relative REPL paths #15464

Merged
merged 1 commit into from
Nov 15, 2016
Merged

Conversation

felixfbecker
Copy link
Contributor

@felixfbecker felixfbecker commented Nov 14, 2016

@bpasero is this the correct usage? Still cannot build + test

#13370

@mention-bot
Copy link

@felixfbecker, thanks for your PR! By analyzing the history of the files in this pull request, we identified @isidorn, @bpasero and @egamma to be potential reviewers.

@isidorn isidorn self-assigned this Nov 14, 2016
@isidorn isidorn added this to the November 2016 milestone Nov 14, 2016
@isidorn isidorn merged commit 62ed382 into microsoft:master Nov 15, 2016
@isidorn
Copy link
Contributor

isidorn commented Nov 15, 2016

Usage looks good to me -> merging in.
Thanks 🍻

@felixfbecker
Copy link
Contributor Author

felixfbecker commented Nov 17, 2016

Absolute paths are broken now

Unable to open 'DefinitionResolver.php': File not found (c:\Users\felix\git\OpenSource\php-language-server\C:\Users\felix\git\OpenSource\php-language-server\src\DefinitionResolver.php).

Seems like the context service cannot resolve paths that are already absolute.

@isidorn
Copy link
Contributor

isidorn commented Nov 18, 2016

@felixfbecker thanks for fixing and testing this feature - you are doing the whole cycle :)

It seems like we should include node 'path' and perform this check https://nodejs.org/api/path.html#path_path_isabsolute_path

Let me know if you are on it, if not no worries I can push it as well.

@felixfbecker
Copy link
Contributor Author

felixfbecker commented Nov 18, 2016

I'm always running on Insiders, and need to click on links in the REPL on a minutely basis 😉
Would be awesome if this could make it into the next stable release.

I though about using path.isAbsolute() too, but that would only work for paths, not file:// URIs.
Other options:

  • Capture the "root" component of the regexp again to check if it exists
  • Modify the context service to accept relative paths

Btw I'm a bit surprised everything works correctly in the output panel, but not in the REPL - shouldn't they share the same code for stuff like this?

@isidorn
Copy link
Contributor

isidorn commented Nov 18, 2016

@felixfbecker yes, perfectly that code would be shared. I could actually look into that refactoring next week.
I would not modify the context service without approval from @bpasero
I would also try to reduce regex magic to a minimum - for simplicity.

@felixfbecker
Copy link
Contributor Author

felixfbecker commented Nov 18, 2016

Another improvement that I would like, currently the terminator at is accepted (for NodeJS exceptions like uncaught Error Bla at index.js:3), PHP exceptions use in, like uncaught Error Bla in index.php:3.

@felixfbecker
Copy link
Contributor Author

And it would be so awesome if the REPL had the same features as the output panel - especially finding and selecting more than one page of text. At the same time the output panel should have the CLI color support of the REPL.

@isidorn
Copy link
Contributor

isidorn commented Nov 18, 2016

The problem with aligning repl and output lies in the fact the repl is using a tree as an implemenation detail and output an editor. You can read up more about this here #11462

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants