GH2008: Added support for typed contexts. #2016
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
patriksvensson
commented
Jan 23, 2018
| { | ||
| internal sealed class CakeDataContextRepository : ICakeDataContextRepository | ||
| { | ||
| private object _data; |
There was a problem hiding this comment.
Allow more than one data context?
patriksvensson
commented
Jan 23, 2018
| /// <summary> | ||
| /// Represent a data context. | ||
| /// </summary> | ||
| public interface ICakeDataContext |
patriksvensson
commented
Jan 23, 2018
| /// <summary> | ||
| /// Represents a data context repository. | ||
| /// </summary> | ||
| public interface ICakeDataContextRepository : ICakeDataContextResolver |
There was a problem hiding this comment.
I really hate this name.
gep13
changed the title
|
Ping @cake-build/cake-team |
patriksvensson
changed the title
patriksvensson
changed the title
|
@cake-build/cake-team This is ready for review. |
|
Relates to #2008 |
devlead
requested changes
Mar 29, 2018
|
Tested with a Cake.Recipe project, looks like this causes serious issues there Tested a few internal advanced Cake scripts and had a few similar issues. |
|
@devlead Yeah, Cake.Recipe will probably need to be updated to not rely on ActionTask anymore. |
|
@patriksvensson what would the alternative be? Cake.Recipe uses ActionTask extensively to allow the manipulation of the Task Graph. |
|
@gep13 |
|
@patriksvensson so is it just a case of replacing ActionTask with CakeTask? |
|
@gep13 Correct. I will add back ActionTask, inherit it from CakeTask and mark it as obsolete. |
bjorkstromm
requested changes
May 22, 2018
| @@ -19,6 +19,7 @@ public sealed class CakeContextFixture | |||
| public IProcessRunner ProcessRunner { get; set; } | |||
| public IRegistry Registry { get; set; } | |||
| public IToolLocator Tools { get; set; } | |||
| public ICakeDataService DataContext { get; set; } | |||
src/Cake.Core/CakeContext.cs
Outdated
| @@ -72,6 +74,7 @@ public sealed class CakeContext : ICakeContext | |||
| ProcessRunner = processRunner; | |||
| Registry = registry; | |||
| Tools = tools; | |||
| Data = data; | |||
src/Cake.Core/CakeEngine.cs
Outdated
| _tasks = new List<CakeTask>(); | ||
| _actions = new CakeEngineActions(dataService); |
src/Cake.Core/CakeEngineActions.cs
Outdated
|
|
||
| public CakeEngineActions(ICakeDataService dataService) | ||
| { | ||
| _dataService = dataService; |
| @@ -35,165 +30,114 @@ public abstract class CakeTask : ICakeTaskInfo | |||
| /// Gets the task's dependencies. | |||
| /// </summary> | |||
| /// <value>The task's dependencies.</value> | |||
| public IReadOnlyList<CakeTaskDependency> Dependencies => _dependencies; | |||
| public List<CakeTaskDependency> Dependencies { get; } | |||
There was a problem hiding this comment.
IList... Or is ISet or ICollection sufficient?
There was a problem hiding this comment.
We could probably change this to an ICollection.
There was a problem hiding this comment.
Changed my mind here, would probably be best to keep compatibility here and use an IList.
There was a problem hiding this comment.
Actually, let's keep this a List. No reason to expose any other type here since it can't be replaced.
|
|
||
| /// <summary> | ||
| /// Gets the tasks that the task want to be a dependency of. | ||
| /// </summary> | ||
| /// <value>The tasks that the task want to be a dependency of.</value> | ||
| public IReadOnlyList<CakeTaskDependency> Dependees => _reverseDependencies; | ||
| public List<CakeTaskDependency> Dependees { get; } |
There was a problem hiding this comment.
IList... Or is ISet or ICollection sufficient?
There was a problem hiding this comment.
We could probably change this to an ICollection.
There was a problem hiding this comment.
Changed my mind here, would probably be best to keep compatibility here and use an IList.
There was a problem hiding this comment.
Actually, let's keep this a List. No reason to expose any other type here since it can't be replaced.
|
|
||
| /// <summary> | ||
| /// Gets the task's criterias. | ||
| /// </summary> | ||
| /// <value>The task's criterias.</value> | ||
| public IReadOnlyList<Func<ICakeContext, bool>> Criterias => _criterias; | ||
| public List<CakeTaskCriteria> Criterias { get; } |
There was a problem hiding this comment.
IList... Or is ISet or ICollection sufficient?
There was a problem hiding this comment.
We could change this to an ICollection since criterias are inclusive (AND based).
There was a problem hiding this comment.
Changed my mind here, would probably be best to keep compatibility here and use an IList.
| Name = name; | ||
| } | ||
| /// <value>The task's actions.</value> | ||
| public List<Func<ICakeContext, Task>> Actions { get; } |
There was a problem hiding this comment.
IList... Or is ISet or ICollection sufficient?
There was a problem hiding this comment.
We could change this to IList, but the collection should keep the order items were added and expose them by index to ensure backwards compatibility. There might be scripts that rely on the order actions are executed (even if that would be an implementation detail).
| _criterias.Add(criteria); | ||
| } | ||
| IReadOnlyList<CakeTaskDependency> ICakeTaskInfo.Dependencies => Dependencies; | ||
| IReadOnlyList<CakeTaskDependency> ICakeTaskInfo.Dependees => Dependees; |
There was a problem hiding this comment.
Hmmm, so the interface exposes IReadOnlyList<T> but the class exposes List<T>. What was the reason for this? Should we instead change the type in the interface? (While we are breaking things 😄 )
There was a problem hiding this comment.
@mholo65 The interface exposes a readonly view of the task and shouldn't be changed for backwards compatibility.
src/Cake.Core/CakeTaskCriteria.cs
Outdated
| /// <param name="message">The criteria's message if skipped.</param> | ||
| public CakeTaskCriteria(Func<ICakeContext, bool> predicate, string message = null) | ||
| { | ||
| Predicate = predicate; |
There was a problem hiding this comment.
Guard against null predicate.
patriksvensson
commented
May 23, 2018
| /// <param name="criteria">The criteria.</param> | ||
| /// <param name="message">The message to display if the task was skipped due to the provided criteria.</param> | ||
| /// <returns>The same <see cref="CakeTaskBuilder"/> instance so that multiple calls can be chained.</returns> | ||
| public static CakeTaskBuilder WithCriteria(this CakeTaskBuilder builder, Func<bool> criteria, string message) |
There was a problem hiding this comment.
Should we change order of criteria and message here?
There was a problem hiding this comment.
Voting says we keep it.
| /// <param name="criteria">The criteria.</param> | ||
| /// <param name="message">The message to display if the task was skipped due to the provided criteria.</param> | ||
| /// <returns>The same <see cref="CakeTaskBuilder"/> instance so that multiple calls can be chained.</returns> | ||
| public static CakeTaskBuilder WithCriteria(this CakeTaskBuilder builder, bool criteria, string message) |
There was a problem hiding this comment.
Should we change order of criteria and message here?
There was a problem hiding this comment.
Voting says we keep it.
| /// <param name="criteria">The criteria.</param> | ||
| /// <param name="message">The message to display if the task was skipped due to the provided criteria.</param> | ||
| /// <returns>The same <see cref="CakeTaskBuilder"/> instance so that multiple calls can be chained.</returns> | ||
| public static CakeTaskBuilder WithCriteria(this CakeTaskBuilder builder, Func<ICakeContext, bool> criteria, string message) |
There was a problem hiding this comment.
Should we change order of criteria and message here?
There was a problem hiding this comment.
Voting says we keep it.
|
Initial and integration tests run fine for me. Cake recipe fails, BUT! It's a very simple fix diff --git a/Cake.Recipe/Content/tasks.cake b/Cake.Recipe/Content/tasks.cake
index d90626f..31d88a3 100644
--- a/Cake.Recipe/Content/tasks.cake
+++ b/Cake.Recipe/Content/tasks.cake
@@ -1,56 +1,56 @@
public class BuildTasks
{
- public CakeTaskBuilder<ActionTask> DupFinderTask { get; set; }
- public CakeTaskBuilder<ActionTask> InspectCodeTask { get; set; }
- public CakeTaskBuilder<ActionTask> AnalyzeTask { get; set; }
- public CakeTaskBuilder<ActionTask> PrintAppVeyorEnvironmentVariablesTask { get; set; }
- public CakeTaskBuilder<ActionTask> UploadAppVeyorArtifactsTask { get; set; }
- public CakeTaskBuilder<ActionTask> ClearAppVeyorCacheTask { get; set; }
- public CakeTaskBuilder<ActionTask> ShowInfoTask { get; set; }
- public CakeTaskBuilder<ActionTask> CleanTask { get; set; }
- public CakeTaskBuilder<ActionTask> DotNetCoreCleanTask { get; set; }
- public CakeTaskBuilder<ActionTask> RestoreTask { get; set; }
- public CakeTaskBuilder<ActionTask> DotNetCoreRestoreTask { get; set; }
- public CakeTaskBuilder<ActionTask> BuildTask { get; set; }
- public CakeTaskBuilder<ActionTask> DotNetCoreBuildTask { get; set; }
- public CakeTaskBuilder<ActionTask> PackageTask { get; set; }
- public CakeTaskBuilder<ActionTask> DefaultTask { get; set; }
- public CakeTaskBuilder<ActionTask> AppVeyorTask { get; set; }
- public CakeTaskBuilder<ActionTask> ReleaseNotesTask { get; set; }
- public CakeTaskBuilder<ActionTask> ClearCacheTask { get; set; }
- public CakeTaskBuilder<ActionTask> PreviewTask { get; set; }
- public CakeTaskBuilder<ActionTask> PublishDocsTask { get; set; }
- public CakeTaskBuilder<ActionTask> CreateChocolateyPackagesTask { get; set; }
- public CakeTaskBuilder<ActionTask> PublishChocolateyPackagesTask { get; set; }
- public CakeTaskBuilder<ActionTask> UploadCodecovReportTask { get; set; }
- public CakeTaskBuilder<ActionTask> UploadCoverallsReportTask { get; set; }
- public CakeTaskBuilder<ActionTask> UploadCoverageReportTask { get; set; }
- public CakeTaskBuilder<ActionTask> CreateReleaseNotesTask { get; set; }
- public CakeTaskBuilder<ActionTask> ExportReleaseNotesTask { get; set; }
- public CakeTaskBuilder<ActionTask> PublishGitHubReleaseTask { get; set; }
- public CakeTaskBuilder<ActionTask> DotNetCorePackTask { get; set; }
- public CakeTaskBuilder<ActionTask> CreateNuGetPackageTask { get; set; }
- public CakeTaskBuilder<ActionTask> CreateNuGetPackagesTask { get; set; }
- public CakeTaskBuilder<ActionTask> PublishMyGetPackagesTask { get; set; }
- public CakeTaskBuilder<ActionTask> PublishNuGetPackagesTask { get; set; }
- public CakeTaskBuilder<ActionTask> InstallReportGeneratorTask { get; set; }
- public CakeTaskBuilder<ActionTask> InstallReportUnitTask { get; set; }
- public CakeTaskBuilder<ActionTask> InstallOpenCoverTask { get; set; }
- public CakeTaskBuilder<ActionTask> TestNUnitTask { get; set; }
- public CakeTaskBuilder<ActionTask> TestxUnitTask { get; set; }
- public CakeTaskBuilder<ActionTask> TestMSTestTask { get; set; }
- public CakeTaskBuilder<ActionTask> TestVSTestTask { get; set; }
- public CakeTaskBuilder<ActionTask> TestFixieTask { get; set; }
- public CakeTaskBuilder<ActionTask> TransifexPullTranslations { get; set; }
- public CakeTaskBuilder<ActionTask> TransifexPushSourceResource { get; set; }
- public CakeTaskBuilder<ActionTask> TransifexPushTranslations { get; set; }
- public CakeTaskBuilder<ActionTask> TransifexSetupTask { get; set; }
- public CakeTaskBuilder<ActionTask> DotNetCoreTestTask { get; set; }
- public CakeTaskBuilder<ActionTask> IntegrationTestTask { get;set; }
- public CakeTaskBuilder<ActionTask> TestTask { get; set; }
- public CakeTaskBuilder<ActionTask> CleanDocumentationTask { get; set; }
- public CakeTaskBuilder<ActionTask> DeployGraphDocumentation {get; set;}
- public CakeTaskBuilder<ActionTask> PublishDocumentationTask { get; set; }
- public CakeTaskBuilder<ActionTask> PreviewDocumentationTask { get; set; }
- public CakeTaskBuilder<ActionTask> ForcePublishDocumentationTask { get; set; }
+ public CakeTaskBuilder DupFinderTask { get; set; }
+ public CakeTaskBuilder InspectCodeTask { get; set; }
+ public CakeTaskBuilder AnalyzeTask { get; set; }
+ public CakeTaskBuilder PrintAppVeyorEnvironmentVariablesTask { get; set; }
+ public CakeTaskBuilder UploadAppVeyorArtifactsTask { get; set; }
+ public CakeTaskBuilder ClearAppVeyorCacheTask { get; set; }
+ public CakeTaskBuilder ShowInfoTask { get; set; }
+ public CakeTaskBuilder CleanTask { get; set; }
+ public CakeTaskBuilder DotNetCoreCleanTask { get; set; }
+ public CakeTaskBuilder RestoreTask { get; set; }
+ public CakeTaskBuilder DotNetCoreRestoreTask { get; set; }
+ public CakeTaskBuilder BuildTask { get; set; }
+ public CakeTaskBuilder DotNetCoreBuildTask { get; set; }
+ public CakeTaskBuilder PackageTask { get; set; }
+ public CakeTaskBuilder DefaultTask { get; set; }
+ public CakeTaskBuilder AppVeyorTask { get; set; }
+ public CakeTaskBuilder ReleaseNotesTask { get; set; }
+ public CakeTaskBuilder ClearCacheTask { get; set; }
+ public CakeTaskBuilder PreviewTask { get; set; }
+ public CakeTaskBuilder PublishDocsTask { get; set; }
+ public CakeTaskBuilder CreateChocolateyPackagesTask { get; set; }
+ public CakeTaskBuilder PublishChocolateyPackagesTask { get; set; }
+ public CakeTaskBuilder UploadCodecovReportTask { get; set; }
+ public CakeTaskBuilder UploadCoverallsReportTask { get; set; }
+ public CakeTaskBuilder UploadCoverageReportTask { get; set; }
+ public CakeTaskBuilder CreateReleaseNotesTask { get; set; }
+ public CakeTaskBuilder ExportReleaseNotesTask { get; set; }
+ public CakeTaskBuilder PublishGitHubReleaseTask { get; set; }
+ public CakeTaskBuilder DotNetCorePackTask { get; set; }
+ public CakeTaskBuilder CreateNuGetPackageTask { get; set; }
+ public CakeTaskBuilder CreateNuGetPackagesTask { get; set; }
+ public CakeTaskBuilder PublishMyGetPackagesTask { get; set; }
+ public CakeTaskBuilder PublishNuGetPackagesTask { get; set; }
+ public CakeTaskBuilder InstallReportGeneratorTask { get; set; }
+ public CakeTaskBuilder InstallReportUnitTask { get; set; }
+ public CakeTaskBuilder InstallOpenCoverTask { get; set; }
+ public CakeTaskBuilder TestNUnitTask { get; set; }
+ public CakeTaskBuilder TestxUnitTask { get; set; }
+ public CakeTaskBuilder TestMSTestTask { get; set; }
+ public CakeTaskBuilder TestVSTestTask { get; set; }
+ public CakeTaskBuilder TestFixieTask { get; set; }
+ public CakeTaskBuilder TransifexPullTranslations { get; set; }
+ public CakeTaskBuilder TransifexPushSourceResource { get; set; }
+ public CakeTaskBuilder TransifexPushTranslations { get; set; }
+ public CakeTaskBuilder TransifexSetupTask { get; set; }
+ public CakeTaskBuilder DotNetCoreTestTask { get; set; }
+ public CakeTaskBuilder IntegrationTestTask { get;set; }
+ public CakeTaskBuilder TestTask { get; set; }
+ public CakeTaskBuilder CleanDocumentationTask { get; set; }
+ public CakeTaskBuilder DeployGraphDocumentation {get; set;}
+ public CakeTaskBuilder PublishDocumentationTask { get; set; }
+ public CakeTaskBuilder PreviewDocumentationTask { get; set; }
+ public CakeTaskBuilder ForcePublishDocumentationTask { get; set; }
}After that adjustment recipes execute fine, this should be well documented in the blog post though as it'll break things for sure. |
|
@patriksvensson your changes have been merged, thanks for your contribution 👍 |
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.
This would remove dependencies on global variables and shared mutable states that make larger Cake script difficult to maintain in some scenarios. Declaring a
Setup<TContextData>should be required to use the context data in the script. If noSetup<TContextData>has been declared, an error should be thrown when the script compiles.There's also some work in this PR to simplify the
CakeTaskBuilder. See explaination below that summarizes why this might be a good idea.Example of API additions:
Declare a context in your Cake script like this:
And now you can use it like this:
Summary from internal discussion with Cake team: