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

When SolutionFilePath is available set in Workspace Solution #177

Merged
merged 1 commit into from
May 4, 2021

Conversation

markrendle
Copy link
Contributor

When creating a Workspace from an AnalyzerManager created from a solution file, this change ensures that CurrentSolution.FilePath is set correctly.

There were some failing integration tests but I think they're due to my environment being different.

return Path.GetFullPath(
Path.Combine(
Path.GetDirectoryName(typeof(ProjectAnalyzerExtensionsFixture).Assembly.Location),
@"..\..\..\..\" + partialPath));
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using Directory.GetParent instead, slashes are different in different operating systems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to change too much, that was existing code refactored into a shared method so I assume it's been working.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@markrendle thanks, keeping the changes limited makes it easier for me to pull this change in without spending a lot of time I don't have right now reviewing it.

@psfinaki I think you've got a very good point though and at some point soon I'll need to do a little auditing of the code to remove slashes wherever I can. That said, I do think the Path family of APIs normalizes slash characters fairly well across platforms which is why this code worked well enough before.

In any case, good enough to merge and release for now :)

@daveaglick
Copy link
Collaborator

Thanks!

@daveaglick daveaglick merged commit 51ba81f into phmonte:main May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants