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

Add support for dotnet sln add file and dotnet sln add folder #45072

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Bartleby2718
Copy link

@Bartleby2718 Bartleby2718 commented Nov 23, 2024

This PR closes #9611.

@baronfel, can you

  • assign the issue to me;
  • assign this PR to me; and
  • review this PR?

Thanks in advance!

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Nov 23, 2024
string args;
if (_inRoot)
{
args = $"--{SlnAddParser.InRootOption.Name} ";
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a bug where there were 4 dashes.
image

]);
}

public static bool IsValidSolutionFolderName(string folderName)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image


private static string TrimProject(string path)
Copy link
Author

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:
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
Didn't check for user options files; should I? If so, what files should I be looking for?

Copy link
Author

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

@Bartleby2718 Bartleby2718 marked this pull request as ready for review November 23, 2024 20:37
@kasperk81
Copy link
Contributor

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 src/Cli/Microsoft.DotNet.Cli.Sln.Internal (reading/writing logic is offloaded to visual studio library https://github.com/microsoft/vs-solutionpersistence) and THEN start adding new feature on that baseline.

@nagilson
Copy link
Member

nagilson commented Nov 25, 2024

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 src/Cli/dotnet/commands/dotnet-sln/add/file/AddFileToSolutionCommand.cs and AddFolderToSolution logic is relatively decoupled from the other logic. I think, once the slnx add code is in, it will be possible to add back that logic in, if you still have interest. I see you've closed #45033.

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.

@edvilme
Copy link
Member

edvilme commented Nov 26, 2024

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

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 dotnet sln add.

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 SolutionModel::AddFolder and SolutionFolderModel::AddFile methods.

Please let us know if you have any questions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dotnet sln add should allow non-project files to be added to the solution
4 participants