(GH-121) Implement ConfigurationHandler #135

gep13 · 2019-03-01T23:03:07Z

To allow for suppressing individual rules.

Relates to #121

gep13 · 2019-03-01T23:04:26Z

@tintoy @mholo65 would either of you be able to have a review of this PR? Thanks!

gep13 · 2019-03-01T23:06:00Z

This allows for configuration similar to the following:

{
  "chocoLanguageServer":{
    "language": {
      "suppressedRules": [
        "CHOCO0002"
      ]
    }
  }
}

NOTE: None of this is fixed in stone, just to show how things can be done.

tintoy · 2019-03-02T01:57:39Z

src/Chocolatey.Language.Server/Handlers/ConfigurationHandler.cs

+            {
+                try
+                {
+                    return (Task<Unit>)OnDidChangeConfiguration(request);


At first glance, I'm wondering whether this cast will ever succeed? OnDidChangeConfiguration returns Task.CompletedTask, not Task<Unit>.

@tintoy Ah, that one... I think I did that just to get it to compile 😄 This will likely need some tidy up.

AdmiringWorm · 2019-03-02T10:44:21Z

So far it looks good to me

mkevenaar

Looks amazing!

- Based on the work in this repository: https://github.com/tintoy/msbuild-project-tools-server - Needed updated due to changes in most recent version of Omnisharp

- Added new interface to provide up to date Configuration - Changed Rule registrations. Use container, rather than reflection - This will mean that each rule has to be registered with container

- Grouping all files into correct folders - Changing namespaces, etc

- Handle the case where there is no element - Diagnostics are placed at top of file

gep13 · 2019-03-02T21:51:14Z

@AdmiringWorm @mkevenaar @tintoy I have done a fair bit of refactoring here, but I think it is all for the greater good.

Refactored so that only one rule per validation
This allowed adding an Id and DocumentationUrl property to each rule
Added ability to suppress a rule. If it is suppressed, the Validate method will never be called on that rule

What are your collective thoughts on these changes?

tintoy · 2019-03-02T22:32:28Z

Looks good to me - getting everything from the DI container will keep things simpler :)

gep13 · 2019-03-02T23:14:18Z

I am starting to play with this:

Trying to update the configuration, to add suppressions for a particular rule. Things don't seem to be working though. Anyone played with this?

bjorkstromm · 2019-03-03T08:10:11Z

@gep13 do you have some code for the above?

gep13 · 2019-03-03T08:17:40Z

@mholo65 I have it locally, but I was struggling to get things working the way I want it to. I am thinking about splitting that work into a separate PR.

mkevenaar · 2019-03-03T08:28:55Z

Have you looked how https://github.com/DavidAnson/vscode-markdownlint did it? It has a MIT license

Also I think the CHOCO0### should not be ignorable, as they are a requirement.

gep13 · 2019-03-03T09:07:25Z

@mkevenaar said...
Have you looked how https://github.com/DavidAnson/vscode-markdownlint did it? It has a MIT license

Yes. I have been using that repository, as well as this one https://github.com/Jason-Rev/vscode-spell-checker, as a reference for what I am trying to achieve.

@mkevenaar said...
Also I think the CHOCO0### should not be ignorable, as they are a requirement.

I have had the same thought. While it is correct to say that this is a requirement for pushing to Chocolatey.org, it may not be a requirement when pushing packages internally. As such, I think I am going to leave the ability to suppress a requirement, but also put in another configuration parameter named something like allowSuppressionOfRequirements which will control whether this can be done or not.

AdmiringWorm · 2019-03-03T09:52:47Z

src/Chocolatey.Language.Server/Program.cs

@@ -39,6 +45,15 @@ static void ConfigureServices(IServiceCollection services)
        {
            services.AddSingleton<BufferManager>();
            services.AddSingleton<DiagnosticsHandler>();
+            services.AddSingleton<Configuration>();
+            services.AddSingleton<INuspecRule, CopyrightAndAuthorFieldShouldntContainEmailRequirement>();


I'm all for having the services registered and using with dependency inject,
but wouldn't it be better to have each type registered dynamically instead of having to add them every time a rule gets added.

Something like:

var typeLocator = new TypeLocator(); foreach (var nuspecRule in typeLocator.GetTypesThatInheritOrImplement<INuSpecRule>()) { services.AddSingleton(typeof(INuSpecRule), nuspecRule); }

I know you deleted the TypeLocater, but I still think it could be useful to have.

AdmiringWorm · 2019-03-03T10:08:13Z

Refactored so that only one rule per validation

I think that makes sense, and I'm all for it.

This allowed adding an Id and DocumentationUrl property to each rule

Perhaps adding the validation type as a property could also be useful?

Added ability to suppress a rule. If it is suppressed, the Validate method will never be called on that rule

👍

- This will be a breaking change - Allows for future expansion, and better grouping of related properties - Removed mention of MSBuild

- To help with the registration of all validation rules

- So that it is easy to identify rule types

gep13 · 2019-03-03T22:41:07Z

@AdmiringWorm @mkevenaar I have implemented the suggestions that were left in this PR, and I have opened up some follow up issues here:

https://github.com/gep13/chocolatey-vscode/issues/140
https://github.com/gep13/chocolatey-vscode/issues/139

I am going to go ahead and merge this in, as it impacts on a lot of different sections, and I don't want to hold up any further work. I have tested most of the changes, and it seems to be working fine. If there are any issues found, we can correct them as we go.

As I mention in the related issue, this will be a breaking change, as I had to refactor the default configuration that is contributed by the extension.

Let me know if you have any questions about anything.

gep13 · 2019-03-06T07:22:28Z

@all-contributors please add @tintoy for ideas, review

allcontributors · 2019-03-06T07:22:35Z

@gep13

I've put up a pull request to add @tintoy! 🎉

gep13 requested review from mkevenaar, AdmiringWorm and steviecoaster March 1, 2019 23:03

gep13 mentioned this pull request Mar 1, 2019

Question about IDidChangeConfigurationHandler OmniSharp/csharp-language-server-protocol#124

Closed

tintoy reviewed Mar 2, 2019

View reviewed changes

mkevenaar reviewed Mar 2, 2019

View reviewed changes

gep13 added 3 commits March 2, 2019 19:42

(GH-121) Initial PoC

0421d78

(GH-121) Refactoring of ConfigurationHandler

122b1f1

- Based on the work in this repository: https://github.com/tintoy/msbuild-project-tools-server - Needed updated due to changes in most recent version of Omnisharp

(GH-121) Refactoring implementation

36f6f3b

- Added new interface to provide up to date Configuration - Changed Rule registrations. Use container, rather than reflection - This will mean that each rule has to be registered with container

gep13 force-pushed the feature/GH-121 branch from 064b563 to 36f6f3b Compare March 2, 2019 19:50

gep13 added 6 commits March 2, 2019 19:51

(GH-121) Corrected rebase

aa70aa5

(maint) Formatting

95b274c

(maint) General refactoring

73610f2

- Grouping all files into correct folders - Changing namespaces, etc

(GH-121) Refactor to single rule per file

c6112ee

(GH-121) Only run validation is not suppressed

28f0f02

- Handle the case where there is no element - Diagnostics are placed at top of file

(GH-121) Changed method signature

627a941

AdmiringWorm reviewed Mar 3, 2019

View reviewed changes

gep13 added 2 commits March 3, 2019 22:14

(GH-121) Refactored contributed configuration

cacd8a5

- This will be a breaking change - Allows for future expansion, and better grouping of related properties - Removed mention of MSBuild

(GH-121) Re-instated the TypeLocator

66b984c

- To help with the registration of all validation rules

(GH-121) Added ValidationType to INuspecRule

2407102

- So that it is easy to identify rule types

gep13 changed the title ~~[WIP] Implement ConfigurationHandler~~ Implement ConfigurationHandler Mar 3, 2019

gep13 merged commit adf8b11 into develop Mar 3, 2019

mergify bot deleted the feature/GH-121 branch March 3, 2019 22:42

gep13 changed the title ~~Implement ConfigurationHandler~~ (GH-121) Implement ConfigurationHandler Mar 6, 2019

allcontributors bot mentioned this pull request Mar 6, 2019

docs: add tintoy as a contributor #152

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(GH-121) Implement ConfigurationHandler #135

(GH-121) Implement ConfigurationHandler #135

gep13 commented Mar 1, 2019

gep13 commented Mar 1, 2019

gep13 commented Mar 1, 2019

tintoy Mar 2, 2019

gep13 Mar 2, 2019

AdmiringWorm commented Mar 2, 2019

mkevenaar left a comment

gep13 commented Mar 2, 2019

tintoy commented Mar 2, 2019

gep13 commented Mar 2, 2019

bjorkstromm commented Mar 3, 2019

gep13 commented Mar 3, 2019

mkevenaar commented Mar 3, 2019 •

edited

Loading

gep13 commented Mar 3, 2019

AdmiringWorm Mar 3, 2019

AdmiringWorm commented Mar 3, 2019

gep13 commented Mar 3, 2019

gep13 commented Mar 6, 2019

allcontributors bot commented Mar 6, 2019

(GH-121) Implement ConfigurationHandler #135

(GH-121) Implement ConfigurationHandler #135

Conversation

gep13 commented Mar 1, 2019

gep13 commented Mar 1, 2019

gep13 commented Mar 1, 2019

tintoy Mar 2, 2019

Choose a reason for hiding this comment

gep13 Mar 2, 2019

Choose a reason for hiding this comment

AdmiringWorm commented Mar 2, 2019

mkevenaar left a comment

Choose a reason for hiding this comment

gep13 commented Mar 2, 2019

tintoy commented Mar 2, 2019

gep13 commented Mar 2, 2019

bjorkstromm commented Mar 3, 2019

gep13 commented Mar 3, 2019

mkevenaar commented Mar 3, 2019 • edited Loading

gep13 commented Mar 3, 2019

AdmiringWorm Mar 3, 2019

Choose a reason for hiding this comment

AdmiringWorm commented Mar 3, 2019

gep13 commented Mar 3, 2019

gep13 commented Mar 6, 2019

allcontributors bot commented Mar 6, 2019

mkevenaar commented Mar 3, 2019 •

edited

Loading