Create supported and documented NuGet package(s)

[mungojam]

Fixes #238 Fixes #185

[mungojam]

See below for the current API to do a migration once we have the package available. I'm not familiar with serilog, so I've guessed how to replicate what the console version does.

It would be good to provide some defaults for the logging and the glob processor so that we can cut down the amount of code that is required to do a migration with the library. I haven't got my head round the glob processor and I doubt most people will need to customise it.

Perhaps we could also move the delete and checkout operations into the ConversionOptions so that they can be overridden while still making use of the convenient Facility class. noBackup could also be moved inside ConversionOptions. Or if we want ConversionOptions to not be polluted with these, then all 3 options (noBackup, delete and checkout) could be moved into WriterOptions or something similar.

Log.Logger = new LoggerConfiguration()
                .Enrich.FromLogContext()
                .Enrich.WithDemystifiedStackTraces()
                .MinimumLevel.ControlledBy(new LoggingLevelSwitch())
                .WriteTo.Console()
                .CreateLogger();

 PatternProcessor globProcessor = (converter, pattern, callback, self) =>
        {
            Log.Verbose("Falling back to globbing");
            self.DoProcessableFileSearch();
            var glob = Glob.Parse(pattern);
            Log.Verbose("Parsed glob {Glob}", glob);
            foreach (var (path, extension) in self.Files)
            {
                if (!glob.IsMatch(path)) continue;
                var file = new FileInfo(path);
                callback(file, extension);
            }

            return true;
        };

var genericLogger = new Serilog.Extensions.Logging.SerilogLoggerProvider().CreateLogger(nameof(Serilog));

facility = new Facility(genericLogger, globProcessor);

facility.ExecuteMigrate(new [] {"SomeSolution.sln"}, noBackup: true, conversionOptions: new ConversionOptions());

[andrew-boyarshin]

@mungojam yeah, basically waiting for NuGet keys here.

About the API. I'd take the separate ProjectWriteOptions (WriterOptions? maybe some voting is required) approach. Internals of globbing are supposed to be a "blackbox", but maybe an additional public API should be added to change glob root directory, that is hardcoded here: https://github.com/hvanbakel/CsprojToVs2017/blob/f7ed1bc291d72a8a66840aee6525e6b40d0d5191/Project2015To2017/Facility.cs#L96-L102

PatternProcessor globProcessor is not required when no glob patterns are expected in the input paths array. It can be omitted in Facility constructor. But it might be sound to move this kind of default implementation to Project2015To2017 package too, just in case one needs to use it but doesn't want to pay extra attention to this detail. Although I'm not sure it should be included by default in the list of input path list processors, I believe user should state his intention explicitly in this case. I believe even the 2nd default one (directoryProcessor) is not necessary in the default processor list.

[mungojam]

@andrew-boyarshin thanks for the clarification, I hadn't spotted globProcessor was optional. That makes it simpler:

Log.Logger = new LoggerConfiguration()
                .Enrich.FromLogContext()
                .Enrich.WithDemystifiedStackTraces()
                .MinimumLevel.ControlledBy(new LoggingLevelSwitch())
                .WriteTo.Console()
                .CreateLogger();

var genericLogger = new Serilog.Extensions.Logging.SerilogLoggerProvider().CreateLogger(nameof(Serilog));

facility = new Facility(genericLogger);

facility.ExecuteMigrate(new [] {"SomeSolution.sln"}, noBackup: true, conversionOptions: new ConversionOptions());

If the pre and post transforms in ConversionOptions give enough flexibility, then this should work fine. I think when I tried something similar with this newer version, I could no longer achieve what I wanted (messing with the assembly info version prior to conversion), so I had to overwrite the default TransformationSet, which is still possible by going a bit lower level:

var converter = new ProjectConverter(Logger, Vs15TransformationSet.Instance, conversionOptions);

var solution = SolutionReader.Instance.Read("SomeSolution.sln", Logger);

var convertedProjects = converter.ProcessSolutionFile(solution);

var writer = new ProjectWriter(Logger, new ProjectWriteOptions());
foreach (var project in projects)
{
    writer.TryWrite(project);
}

I'll have a play with a few examples. I may not get around to any refactoring of options after all at this stage.

[hvanbakel]

Regarding the nuget keys I can get to them tomorrow but we can just move forward at this point appveyor isn't configured to publish anything yet so I think that should be fine. I'll take care of both of those.

On Sat, Apr 27, 2019, 08:42 Mark Adamson notifications@github.com wrote:

@andrew-boyarshin https://github.com/andrew-boyarshin thanks for the clarification, I hadn't spotted globProcessor was optional. That makes it simpler:

Log.Logger = new LoggerConfiguration() .Enrich.FromLogContext() .Enrich.WithDemystifiedStackTraces() .MinimumLevel.ControlledBy(new LoggingLevelSwitch()) .WriteTo.Console() .CreateLogger(); var genericLogger = new Serilog.Extensions.Logging.SerilogLoggerProvider().CreateLogger(nameof(Serilog)); facility = new Facility(genericLogger); facility.ExecuteMigrate(new [] {"SomeSolution.sln"}, noBackup: true, conversionOptions: new ConversionOptions());

If the pre and post transforms in ConversionOptions give enough flexibility, then this should work fine. I think when I tried something similar with this newer version, I could no longer achieve what I wanted (messing with the assembly info version prior to conversion), so I had to overwrite the default TransformationSet, which is still possible by going a bit lower level:

var converter = new ProjectConverter(Logger, Vs15TransformationSet.Instance, conversionOptions); var solution = SolutionReader.Instance.Read("SomeSolution.sln", Logger); var convertedProjects = converter.ProcessSolutionFile(solution); var writer = new ProjectWriter(Logger, x => x.Delete(), _ => { });foreach (var project in projects) { writer.TryWrite(project, doBackup: false); }

I'll have a play with a few more examples. I may not get around to any refactoring of options after all at this stage.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/hvanbakel/CsprojToVs2017/pull/243#issuecomment-487296267, or mute the thread https://github.com/notifications/unsubscribe-auth/AA7QKPBBXEUAETCVAINY2XTPSRX5JANCNFSM4HIVVMRQ .

[mungojam]

There's no great hurry to publish from my point of view. I haven't even tried using the packages locally yet

[andrew-boyarshin]

@mungojam yeah, good idea to expose option to override the TransformationSet. Then we can deprecate custom pre- & post-transformations.

[mungojam]

Facility looks like the right level to focus on and is now pretty simple to call with different options or defaults. I wonder if we can rename it to something more like ProjectConverter (I know that's already taken, so it would probably need to be something else).

This is how we're currently looking

//...some logging setup still to be sorted out

var facility = new Facility(genericLogger);

facility.ExecuteMigrate(new [] {"SomeSolution.sln"}, Vs15TransformationSet.Instance, new ConversionOptions(), new ProjectWriteOptions());

[andrew-boyarshin]

I'd revert the move of Facility and related classes to Migrate2017. It really makes little sense to be there and imposes additional hardwired dependency. P.S. I couldn't push JB reformatted sources for another PR today, will do it tomorrow.

[mungojam]

I'd revert the move of Facility and related classes to Migrate2017. It really makes little sense to be there and imposes additional hardwired dependency.

I should explain my reasoning here. As it was, to use Facility, I would have needed to add a reference to the Project2015to2017 package, which I believe we are saying is the legacy package and currently has different versioning. In master, that project then depends on the other projects. I understand the point about hard coding to Migrate2017, but it does reference that project as it is so it couldn't be moved to a lower level in the dependencies.

As an alternative to what I have done, I'm happy to have an umbrella package which is effectively the original Project2015to2017 package which we won't then retire (but can re-release under another name). I'll then revert the move and we'll mainly publicise that package which will then just depend on the others. What do you think?

[hvanbakel]

Regarding api keys, we should be able to reuse this one for any library projects: https://github.com/hvanbakel/CsprojToVs2017/blob/f7ed1bc291d72a8a66840aee6525e6b40d0d5191/appveyor.yml#L40

[mungojam]

Is it safe to have that public in the code? I don't know much nuget publishing

[hvanbakel]

It's not the actual key but an encrypted token. So yes, that is fine you cannot copy that to a different account for example.

[mungojam]

I've nearly got my own use of this package back to it's previous functionality with this new version. One issue I am hitting though is the ordering of transformations which is getting altered undesirably.

I am passing in a set of pre and post transforms as per the updated readme, but the tool is putting some of the basic in-built transforms before my own ones even though I provided them earlier in the list. I need some transforms to run before the default ones so that I can do things like removing attributes from AssemblyInfo that I want to deal with myself. I end up doing that too late as it is now so the AssemblyInfo file doesn't get marked for deletion.

See my pre-transforms getting wedged in the middle of the default ones when the converter gets the actual list to iterate through:

[andrew-boyarshin]

@mungojam did you set early execution target moment on your transformations?

[andrew-boyarshin]

For some time now we have an advanced transformation graph, with dependencies and 3 stages: early, generic (normal) and late. All transformations from set are split into 3 groups and then sorted topologically within their stage with dependencies as graph edges. You can see the examples of both specifying dependencies and selecting target execution moment in some standard transformations.

[mungojam]

did you set early execution target moment on your transformations?

I have now and that has fixed the issue, but it makes the package API quite unintuitive I think as there is nothing forcing me to implement ITransformationWithTargetMoment in the transforms that I pass in. It felt odd to have to change all 8 of my custom transforms to implement it and add the property when I felt I was clearly specifying that some should run first and some should run last based on the order I passed them in.

It makes me think that we should keep the pre and post transforms idea but make it work, by stopping them being used when ExecuteMigrate does the DoAnalysis step at the end. That is what was causing problems with pre and post transform since they were getting run twice, once during migration and once during the follow-up analysis.

Alternatively we do something to make the topological sorting work in a more natural way when a user just wants to add pre and post transforms.

[hvanbakel]

I would go for the documentation approach, I believe that the extension through the nuget package case is rare. The upside is that this gives you a lot of flexibility at the cost of a bit more complexity.

I think forking is way more common

[mungojam]

I'd revert the move of Facility and related classes to Migrate2017. It really makes little sense to be there and imposes additional hardwired dependency.

@andrew-boyarshin, @hvanbakel, this is the one remaining question from my point of view. Can you see what I've replied in https://github.com/hvanbakel/CsprojToVs2017/pull/243#issuecomment-487399809 and let me know what you think should be the way forward.

Hope we can get this merged soon and then we'll have a stable package that I and others can use

[andrew-boyarshin]

@mungojam I'm fine with keeping Project2015To2017 meta package as not deprecated, but an umbrella project (with dependencies on all .MigrateXXXX.Library and containing the Facility class).

[mungojam]

Why rename Facility? Not blocker though. The other comment is the one.

I felt that Facility was a bit too generic a name for the main entry class that somebody would want to create. We could argue it's clear from the namespace. However, it will generally just get created once so I think it's ok to be a bit more verbose.