-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add support for dotnet sln add file
and dotnet sln add folder
#45072
base: main
Are you sure you want to change the base?
Add support for dotnet sln add file
and dotnet sln add folder
#45072
Conversation
string args; | ||
if (_inRoot) | ||
{ | ||
args = $"--{SlnAddParser.InRootOption.Name} "; |
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.
]); | ||
} | ||
|
||
public static bool IsValidSolutionFolderName(string folderName) |
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.
|
||
private static string TrimProject(string 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.
Inlined
fullFilePath); | ||
}).ToList(); | ||
|
||
// Perform the same validations as Visual Studio: |
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 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.
Moved inside the add
directory
in case you weren't aware of slnx work, the timing is critical here. #44570 needs to go in for next release: 9.0.200 (by feb) and having this pr up means one of you is going to have massive merge conflicts to address. probably best to hold off this one. let #44570 go forward in peace, let slnx support light up in dotet-sln-remove command, clean up the entire SlnFile project |
It's clear that a tremendous amount of work went into this PR. Thank you, and thank you for engaging with our team on the issue before getting started on it. As Kasper mentioned, there is another PR in parallel which is rewriting most of this logic, and I'm sorry that this wasn't caught earlier and someone stopped you. We are happy to take this feature work, but Kasper is right in that we will have to prioritize SLNX file support first. The news there is: a lot of the refactor you did on the slnAdd code will likely not be used. But I looked at your code and the sln add code, and luckily it seems your The next thing I will say is that, the new library for SLNX and SLN support I believe has a method you can call to help add folders or files to the solution, so it might be worth calling that official API. @edvilme may be able to point you to that, if it exists; I have heard of such a thing but not done much research on that part of the API myself. I think some of the logic will still have to be done in the SDK like you've done here -- the API, for example, does not do some of the leg work for adding a project to a solution, which the aforementioned PR is working on; which again might be another opportunity to re-use a lot of the changes listed here. Another thing I will note is that @/baronfel has had a lot of work come up in conferences, etc; engagement might be a bit lower as a lot of people take holiday vacations around this time. But, that might not be as big of a blocker since the slnx add code needs to go in first. Thank you again :) I'm leaving this open for now and we can continue to chat. |
Hi @Bartleby2718 thanks for your contribution and hard work on this. Indeed we are updating our sln commands to use vs-solutionpersistence API's to support the new slnx file format. This includes Good thing is their APIs simplifies a lot of the process for interacting with sln(x) files. If you're interested I would suggest taking a look at #44570 and at the Please let us know if you have any questions |
This PR closes #9611.
@baronfel, can you
Thanks in advance!