Rewrite MSBuild reader to use MSBuild API #1125
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Control usage of new implementation via SPECFLOW_USE_MSBUILDAPI to be able test both implementations side-by-side - Update TechTalk.SpecFlow.Generator to net46 to be able use MSBuild API, and propagate changes where required across projects which depends on the TechTalk.SpecFlow.Generator - Update MSBuild API to 15.6. This change may be not needed, but, I strongly suspect that it will play more nicely with all version of .NET Core up to not yet released .NET Core 2.1 - Add tests on the exclude folder - Change test a bit to take into account that paths `.\filename` and `filename` appear as two items via MSBuild API, and I dont think that duplicate items is desired behavior - Implementation of MSBuildApiProjectReader is just old resurrected implementation using MSBuild API with small changes See SpecFlowOSS#1029
kant2002
force-pushed
the
kant/rewrite-msbuild-reader
branch
from
cd513fd
to
c817b4e
Compare
SabotageAndi
suggested changes
Apr 24, 2018
| @@ -128,7 +128,19 @@ private bool IsNewProjectSystem(XDocument xDocument) | |||
|
|
|||
| public static SpecFlowProject LoadSpecFlowProjectFromMsBuild(string projectFilePath) | |||
| { | |||
| return new MsBuildProjectReader(new GeneratorConfigurationProvider(new ConfigurationLoader()), new MSBuildRelativePathParser()).ReadSpecFlowProject(projectFilePath); | |||
| var configurationProvider = new GeneratorConfigurationProvider(new ConfigurationLoader()); | |||
| var useMSBuildApi = Environment.GetEnvironmentVariable("SPECFLOW_USE_MSBUILDAPI") == "1"; | |||
There was a problem hiding this comment.
I would make the new project reader the default one. Only so do we recognize errors of it.
|
Looks good! Thanks for it! BTW as you are in the topic: For #1033 we need all PackageReferences of the project to find the plugins. |
This is justified by fact that supporting it, means reimplementing whole MSBuild logic, which is not reasonable. Also for loading plugins would be needed read PackageReferences from the projects, and this would be problematic implement correctly. Current implementation is also flawled. See SpecFlowOSS#1102, and wellknown fact that Exclude property on the MSBuild items is not supported
|
@SabotageAndi remove XML based implementation |
SabotageAndi
approved these changes
Apr 25, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
.\filenameandfilenameappear as two items via MSBuild API, and I dont think that duplicate items is desired behaviorCloses #1029
Types of changes
Checklist: