-
Notifications
You must be signed in to change notification settings - Fork 9
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
Optionally get the solution path from the project file #17
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for engaging in this pull request this late. It looks good to me. I have just one nitpick.
@@ -66,6 +66,21 @@ def binary_path(cls) -> str: | |||
else: | |||
return os.path.join(cls.basedir(), "omnisharp", "OmniSharp.exe") | |||
|
|||
@classmethod | |||
def get_solution_arguments(cls) -> List[str]: | |||
view = sublime.active_window().active_view() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this view
variable used? Can it be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see now it's used to get the window()
, but that can just as well be achieved with sublime.active_window()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no guarantee that the active window is the one where server is being initialized though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. This code should go into on_pre_start
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point about the view
variable! That's what happens when you just copy code from one place to another...
About moving the code to on_pre_start
, what do you mean exactly? Moving all this there and storing the solution_file_path
in a member to just read it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the on_pre_start
classmethod you can fill in the final command
using your logic. You have access to the window
there as it is passed in the method. This way you don't have to rely on sublime.active_window()
.
What @rchl mentions about sublime projects not necessarily living alongside the source code is valid. Would it make sense to instead rely on string template substitutions to resolve the .sln file? As in I want to be able to specify "solution_file": "$folder/testconsoleprj.sln"
in my .sublime-project. This removes the magic from what relative paths mean as a bonus. You can use template variable substitution in the ususal way with sublime.expand_variables
and window.extract_variables()
@@ -25,6 +25,35 @@ case, that means that when you open a view with the `source.cs` base scope, inst | |||
|
|||
Configure OmniSharp by running `Preferences: LSP-OmniSharp Settings` from the Command Palette. | |||
|
|||
## Project Setting | |||
|
|||
The server will automatically find the the solution file from the folder you have opened in Sublime. If you have multiple solutions, or your solution is in another folder, you have to specify the solution file you wish to use in a `sublime-project`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The server will automatically find the the solution file from the folder you have opened in Sublime. If you have multiple solutions, or your solution is in another folder, you have to specify the solution file you wish to use in a `sublime-project`. | |
The server will automatically find the solution file from the folder you have opened in Sublime. If you have multiple solutions, or your solution is in another folder, you have to specify the solution file you wish to use in a `sublime-project`. |
|
||
2. Go to `Project -> Save Project As` and save a `YOURPROJECTNAME.sublime-project` in desired location. | ||
|
||
3. Open your `YOURPROJECTNAME.sublime-project` file that should now appear in the sidebar on the left |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can say instead to invoke "Project: Edit Project" from the Command Palette.
"follow_symlinks": true, | ||
"path": "." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is follow_symlinks
relevant to this whole PR at all?
Also, this is assuming that the project is saved in the same directory as the project itself. Many people save projects in some completely different locations. Maybe it's confusing to show all that and you could instead just replace this with a comment like // project folders
?
if project_file is not None: | ||
data = view.window().project_data() | ||
if 'solution_file' in data: | ||
project_dir = os.path.dirname(project_file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't be assuming that the solution file location is relative to the project file location. Project file can be stored anywhere really. I would rather resolve relative location to the location of the (first) project folder.
And if the location is absolute then none of this path joining should be performed because it wont' be correct in that case.
There's a legacy OmniSharp package for Sublime Text 3 that has a very handy feature: It allows you to specify a solution file to use with OmniSharp in your sublime-project.
That package is causing me some issues with Sublime Text 4, so I'm trying to use this one as a replacement, but it's lacking that feature.
This pull request will add this feature here. I used the same syntax as the other package, so they are compatible.
Note: I only tested this on Windows and MacOS, as I don't have any Linux machine at hand right now.