Linter manager #11606
Linter manager #11606
Conversation
|
@SHKnudsen @BogdanZavu @nate-peters @saintentropy @pinzart90 @QilongTang @sm6srw @aparajit-pratap DynamoCore / DynmoCoreWPF is quite large - to write an extension currently you must reference DynamoCore nuget packages. A couple issues here:
Okay, why am I pointing this out now?This work appears to me to be geared almost exclusively to extension authors - which is great, an expansion of our extension API! 🎉 IMO it could serve as a nice starting point for the creation of a Dynamo.Extension (Dynamo.Extension.WPF might one day exist as well) nuget package. My rough proposal is as follows:
thoughts? for some more (and possibly outdated info) please see: |
|
@mjkkirschner let's have a meeting tomorrow about this since making this architectural change in the way Dynamo handles extensions will increase the scope of our work a little bit. |
|
|
||
| namespace Dynamo.Linting | ||
| { | ||
| public class LinterExtensionDescriptor |
There was a problem hiding this comment.
this looks pretty similar to: https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCore/Extensions/ExtensionData.cs#L6
There was a problem hiding this comment.
- does this class need to exist? See the very similarExtensionData class, could this just be a tuple instead? Do we expect this to grow?
- Does this class really need to be public?
There was a problem hiding this comment.
So ExtensionData has a few more things in it and I feel that using it instead of LinterDescriptor will make things more confusing. I guess the most valuable thing of LinterDescriptor is the actual name. I would expect that there are many similar or almost similar simple structs across dynamo. I think we can have a tuple here but I would prefer the named structure instead. Most likely it will not grow or if it will grow it will grow very little.
I think it can be internal.
There was a problem hiding this comment.
I forgot about the view, we need it there (in the other PR ) where the active linter is being set from the UI.
|
@SHKnudsen overall I think this is looking really good! Let's clarify the serialization aspect tomorrow and I think we can merge it. |
|
Hmm, i cant access the SelfServe details, so not sure what this issue is.. |
|
Only 1 test remaining : DynamoCoreWpfTests.SerializationTests.JSONisSameBeforeAndAfterSaveWithDummyNodes MESSAGE: +++++++++++++++++++ |
| /// <param name="verboseLogging">Indicates if detailed descriptions should be logged</param> | ||
| /// <param name="isTestMode">Indicates if current code is running in tests</param> | ||
| /// <param name="fileName">Name of file where the workspace is saved</param> | ||
| [Obsolete("please use the version with linterManager parameter.")] |
There was a problem hiding this comment.
some questions on this:
- have you verified that all these constructors can be replaced by your new constructor? - What I mean is, do they cover any other use cases?
- If a file is deserialized with no linterManager - should null be passed?
There was a problem hiding this comment.
- i would say I've tested this as much as possible, also all different constructors have gotten a new equivalent just with the linterManager param. Before the changes there were 3 ctors, I've add 3 new ones that replicate the old ones but with that new param.
- Essentially the only thing that is serialized from the linter manager is the active linter (name and id) and issue counts (the counts are only for things like GD and player). If there is no active linter when a graph is saved we just serialize
Linting : [], when deserializing all we are doing is setting the active linter to whatever was serialized (if it's available on the manager). The linter manager is created on the DynamoModel, so if a workspace is deserialized without an active linter value we just don't set any active linter on the LinterManager.
* Initial commit * Move event subscription to extension base class + add property changed filter to rules * comment updates * Update LinterExtensionBase.cs * Update InputNodesNotAllowedRule.cs * Add Deactivate method to linterExtensionBase + LinterManager tests * comment updates * Dispose LinterManager * comment updates * Serialize linter manager to dyn file * clear ruleEvaluationResults on workspace change * Remove test extension * Handle GraphRuleEvaluationResults nodeIds changes * Update SerializationConverters.cs * Fix failing serialization test * comment updates * revert changes to test file
* Linter manager (#11606) * Initial commit * Move event subscription to extension base class + add property changed filter to rules * comment updates * Update LinterExtensionBase.cs * Update InputNodesNotAllowedRule.cs * Add Deactivate method to linterExtensionBase + LinterManager tests * comment updates * Dispose LinterManager * comment updates * Serialize linter manager to dyn file * clear ruleEvaluationResults on workspace change * Remove test extension * Handle GraphRuleEvaluationResults nodeIds changes * Update SerializationConverters.cs * Fix failing serialization test * comment updates * revert changes to test file * Linter ViewExtension (#11634) * Initial commit * Move event subscription to extension base class + add property changed filter to rules * comment updates * Update LinterExtensionBase.cs * Update InputNodesNotAllowedRule.cs * Add Deactivate method to linterExtensionBase + LinterManager tests * comment updates * Initial commit * Add issues count to WorkspaceSaving * add scrollviewer * Dispose LinterManager * Make concrete issues internal * summaries on IRuleIssue * comment updates * Update severity code icon * Serialize linter manager to dyn file * clear ruleEvaluationResults on workspace change * Remove test extension * Show names instead of GUIDs * add help doc * clean up * Handle GraphRuleEvaluationResults nodeIds changes * Update LintingViewExtension.csproj * Update LintingViewExtension.csproj * Update SerializationConverters.cs * Fix failing serialization test * comment updates * revert changes to test file * fix available linters binding * add rules back in test ext * comment updates * remove output path * Update LinterManagerTests.cs * comment updates * hide LinterViewExtension and LinterExtensionBase * Update LinterViewModel.cs * if statements * remove Prism * clean up
* Linter manager (#11606) * Initial commit * Move event subscription to extension base class + add property changed filter to rules * comment updates * Update LinterExtensionBase.cs * Update InputNodesNotAllowedRule.cs * Add Deactivate method to linterExtensionBase + LinterManager tests * comment updates * Dispose LinterManager * comment updates * Serialize linter manager to dyn file * clear ruleEvaluationResults on workspace change * Remove test extension * Handle GraphRuleEvaluationResults nodeIds changes * Update SerializationConverters.cs * Fix failing serialization test * comment updates * revert changes to test file * Linter ViewExtension (#11634) * Initial commit * Move event subscription to extension base class + add property changed filter to rules * comment updates * Update LinterExtensionBase.cs * Update InputNodesNotAllowedRule.cs * Add Deactivate method to linterExtensionBase + LinterManager tests * comment updates * Initial commit * Add issues count to WorkspaceSaving * add scrollviewer * Dispose LinterManager * Make concrete issues internal * summaries on IRuleIssue * comment updates * Update severity code icon * Serialize linter manager to dyn file * clear ruleEvaluationResults on workspace change * Remove test extension * Show names instead of GUIDs * add help doc * clean up * Handle GraphRuleEvaluationResults nodeIds changes * Update LintingViewExtension.csproj * Update LintingViewExtension.csproj * Update SerializationConverters.cs * Fix failing serialization test * comment updates * revert changes to test file * fix available linters binding * add rules back in test ext * comment updates * remove output path * Update LinterManagerTests.cs * comment updates * hide LinterViewExtension and LinterExtensionBase * Update LinterViewModel.cs * remove unused test files * Fix pr comments (#11677) * Linter manager (#11606) * Initial commit * Move event subscription to extension base class + add property changed filter to rules * comment updates * Update LinterExtensionBase.cs * Update InputNodesNotAllowedRule.cs * Add Deactivate method to linterExtensionBase + LinterManager tests * comment updates * Dispose LinterManager * comment updates * Serialize linter manager to dyn file * clear ruleEvaluationResults on workspace change * Remove test extension * Handle GraphRuleEvaluationResults nodeIds changes * Update SerializationConverters.cs * Fix failing serialization test * comment updates * revert changes to test file * Linter ViewExtension (#11634) * Initial commit * Move event subscription to extension base class + add property changed filter to rules * comment updates * Update LinterExtensionBase.cs * Update InputNodesNotAllowedRule.cs * Add Deactivate method to linterExtensionBase + LinterManager tests * comment updates * Initial commit * Add issues count to WorkspaceSaving * add scrollviewer * Dispose LinterManager * Make concrete issues internal * summaries on IRuleIssue * comment updates * Update severity code icon * Serialize linter manager to dyn file * clear ruleEvaluationResults on workspace change * Remove test extension * Show names instead of GUIDs * add help doc * clean up * Handle GraphRuleEvaluationResults nodeIds changes * Update LintingViewExtension.csproj * Update LintingViewExtension.csproj * Update SerializationConverters.cs * Fix failing serialization test * comment updates * revert changes to test file * fix available linters binding * add rules back in test ext * comment updates * remove output path * Update LinterManagerTests.cs * comment updates * hide LinterViewExtension and LinterExtensionBase * Update LinterViewModel.cs * if statements * remove Prism * clean up Co-authored-by: BogdanZavu <zavu_bogdan@yahoo.com>
Purpose
Still a few things to do here, but opening this as a draft for now so its easier to discuss.
This PR does:
LinterRules static eventRuleEvaluated, if the result is failed it gets stored in the linter manager and if it passes it will be removed.LinterRulebase class, currently there is an implementation forGraphandNoderules, which extensions can use to make their own rules.LinterExtensionBasebase class which new Linter extensions needs to inherit from. The base class handles evaluation of Rules when they are requested by the managerDeclarations
Check these if you believe they are true
*.resxfilesReviewers
@saintentropy @nate-peters @BogdanZavu
FYIs
TestLinterExtensionis only here for testing purposes and will be removed before merging. The test extension should go into the DynamoSamples project at some point