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

JsonPropertyPostAction: allow to create file and path #41959

Merged
merged 4 commits into from
Jul 9, 2024

Conversation

Evangelink
Copy link
Member

We would like to promote MSTest.Sdk as the default recommendation for MSTest test templates as part of .NET 9 and it seems a lot more convenient to handle the version through the global.json file rather than a local version.

To do so, we need to be able to either add or edit a global.json file but there is no easy way to do so. The best match would be to use the Add property to an existing json file post-action but it doesn't allow file creation nor even path creation.

I had a chat with @YuliiaKovalova and @baronfel who suggested that I work on a PR improving the post-action.

@Evangelink Evangelink requested a review from a team as a code owner July 3, 2024 15:46
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-dotnet new the item is related to dotnet new command untriaged Request triage from a team member labels Jul 3, 2024
Comment on lines +16 to +17
private const string AllowFileCreationArgument = "allowFileCreation";
private const string AllowPathCreationArgument = "allowPathCreation";
Copy link
Member Author

Choose a reason for hiding this comment

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

Split into 2 properties to allow users to be able to add the missing property path without forcing requirement to allow file creation.

There is some potential discussion here because it's not really making sense to allow file creation and not allow path creation so we could maybe have some kind of enum instead (e.g. CreationMode with None being the default, Path and File or FileAndPath). We could also keep the boolean and say the allowPathCreation is assumed to be true when allowFileCreation is set to true.

}

return node;
}

private static IReadOnlyList<string> FindFilesInCurrentProjectOrSolutionFolder(
private static string[] FindFilesInCurrentFolderOrParentFolder(
Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed to better match actual logic in place.

IPhysicalFileSystem fileSystem,
string startPath,
string matchPattern,
Func<string, bool>? secondaryFilter = null,
Copy link
Member Author

Choose a reason for hiding this comment

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

Unused.

IPhysicalFileSystem fileSystem,
string startPath,
string matchPattern,
Func<string, bool>? secondaryFilter = null,
int maxAllowedAboveDirectories = 250)
Copy link
Member Author

Choose a reason for hiding this comment

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

Was forced to 1 on the only calling place.

}

directory = Path.GetPathRoot(directory) != directory ? Directory.GetParent(directory)?.FullName : null;
numberOfUpLevels++;
}
while (directory != null && numberOfUpLevels <= maxAllowedAboveDirectories);
while (directory != null && numberOfUpLevels <= 1);
Copy link
Member Author

Choose a reason for hiding this comment

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

It'd be good to have a better walk-up logic but I couldn't think of anything that would work for all/most cases.

  1. Walking up to first project found is problematic for templates adding project as we would never walk-up.
  2. Walking up to first sln found can lead to walking up to root if no sln exists
  3. Walking up to repo root is hard because we have nothing to understand where to stop (is it git folder? is it something else?).

@Evangelink
Copy link
Member Author

Error seems unrelated to this PR: error : Repository asset manifest files don't exist. [D:\a\_work\1\vmr\repo-projects\fsharp.proj]

@Evangelink
Copy link
Member Author

Ping @dotnet/templating-engine-maintainers

@YuliiaKovalova
Copy link
Member

Looks good to me!

@Evangelink Evangelink requested a review from joeloff July 8, 2024 08:34
@Evangelink
Copy link
Member Author

@MiYanni Could you please have a look at this PR?

Copy link
Member

@MiYanni MiYanni left a comment

Choose a reason for hiding this comment

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

I think the 2 properties is fine for the time being. It gets the functionality across without additional work in the code. Everything else looks good.

@Evangelink
Copy link
Member Author

I keep getting different issues with the VMR leg (at every update) but they are all unlinked from my change. Is there a way to force merge?

@Evangelink Evangelink merged commit dfe3107 into dotnet:main Jul 9, 2024
37 checks passed
@Evangelink Evangelink deleted the post-process-modify-json branch July 9, 2024 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-dotnet new the item is related to dotnet new command untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants