cflint / CFLint

Static code analysis for CFML (a linter)
BSD 3-Clause "New" or "Revised" License
173 stars 84 forks source link

Document .cflintrc schema #352

Closed KamasamaK closed 7 years ago

KamasamaK commented 7 years ago

There are examples of the formats for .cflintrc and .cflintrc.xml in the wiki, but the schemas should be properly documented.

I think it would also be a good idea for .cflintrc.json to be on JSON Schema Store if no one here foresees that undergoing any breaking changes.

TheRealAgentK commented 7 years ago

Yeah, this needs to be cleaned up, too. I'm trying to make sense of some aspects of the config system myself at the moment and I don't see why we need to have

I see there's a mechanism of nesting configurations implemented, but I can't seem to get my head around what it taking precedence over what in there. Help please, @ryaneberly :)

TheRealAgentK commented 7 years ago

Re .cflintrc:

{
  "output" : [ ],
  "rule" : [ ],
  "excludes" : [ ],
  "includes" : [ ],
  "inheritParent" : false,
  "inheritPlugins" : true
}

This seems to be kind of the structure at the moment. inheritPlugins will be deprecated in 1.2 and a warning message will be output. In 1.3 I'll remove it and people's code will break if they don't remove the config setting.

KamasamaK commented 7 years ago

I'm not sure what you mean by "global config file". It appears to also have a rule and excludes array, so I assumed it was the .cflintrc.xml format. If you place it in the root it would functionally become global, though like Application.cfc any file that appears closer in the hierarchy would take precedence.

I found that the JSON format was added as a result of Issue #103. I don't really see the benefit in maintaining more than one configuration format and would be fine if .cflintrc.xml were deprecated. Most other linters only maintain one config format, but none of the ones I've used had XML. It's mostly JSON, with some CSON or YAML, and even INI.

TheRealAgentK commented 7 years ago

I mean this: https://github.com/cflint/CFLint/wiki/Include-Exclude-Rules-Using-External-XML-File

Is that .cflintrc.xml ???

And yes - I fully agree. Going forward there should be one configuration format for file-based configuration and ideally not XML.

I think we should maybe even deprecate the XML format for 1.2.0 and get rid of it in 2.0 the latest.

KamasamaK commented 7 years ago

The formats look compatible to me based on the examples. After digging into the code, it does look like there's at least one other configuration format. The only two ways I know of that the config files would be read is during a direct scan or passed through the command line. It looks like loadConfig() accepts CFLintPluginInfo or CFLintConfig whereas the scan() is only CFLintConfig. I haven't dug too deeply in, but CFLintPluginInfo does appear to be a different file format that also has the rules list but no excludes so it doesn't match the one you linked.

TheRealAgentK commented 7 years ago

Yeah, sorry @KamasamaK - I struggle with it myself at the moment. I think we need @ryaneberly to have a look at it and provide some assistance :)

ryaneberly commented 7 years ago

ha. I struggle with this myself. It's an outcome of some growing pains -- I will take a look though. @TheRealAgentK I agree with deprecating the xml format.

ryaneberly commented 7 years ago

@TheRealAgentK , I updated the readme a bit. You are on the right track.
The short history is that the initial global configuration was XML, and the folder level configuration was based on it. Migration to JSON made sense -- we need to deprecate all xml config support to keep things simple.

Some of the items in the .cflintrc I doubt will ever be used. I wonder if we should skip documenting output, rule, inheritParent, and inheritPlugins - and leave them out of the example .cflintrc and schema. Includes and excludes are the ones that have the most meaning. The rule attribute could stay, but i suspect it will just cause questions and not add value. I've never used it at the folder level.

KamasamaK commented 7 years ago

I will have to verify what exactly each setting does to give better feedback, so some of this may already exist. Since we're talking about potential changes, in addition to just a list of rules to include or exclude, I think some configuration to do the following would also be helpful:

  1. Allow additional configuration for particular rules. For example, VAR_TOO_LONG should be able to take a maxLength that overrides the default.

  2. Engine. I believe that CFLint only officially supports ACF currently. I think that having the ability to define the version of ACF might be useful for some cases. Also, Lucee has become very popular in the CFML community so that could also be an option. Each rule would then need to have engine support integrated, though, perhaps with a default. I think most (all?) of the current rules would apply to all of the modern engines, but this could be a way to have rules that suggest using newer language features (e.g. the ModernSyntax rule group).

  3. Change the severity of a particular rule. This would mostly be useful when combined with the next suggestion.

  4. Allow include/exclude of the severity level instead of just individual rules

  5. Do rules have an inherent associated group/category? The Rule Groups and Built In Rules pages appear to have them grouped in some way. Also, some output formats have category, though I'm not sure it's used for anything. That could be another way to filter.

  6. An ignore pattern would be useful when linting directories. Some others have this in a separate file (e.g. .cflintignore).

All of these suggestions were taken from other popular linters. Many of those suggestions exceed the scope of just the configuration file, especially Engine, but I wanted this to be a jumping off point for discussion.

ryaneberly commented 7 years ago

The rule attribute supports #1 and #3 to a degree.

I like the way you are thinking.

TheRealAgentK commented 7 years ago

@KamasamaK This last post of yours should be its own issue with a title "Config 2.0" and milestone 2.0, I think.

KamasamaK commented 7 years ago

So per Issue #315 and #356, does that mean that the default config file should be the following?

{
  "rule" : [ ],
  "excludes" : [ ],
  "includes" : [ ],
  "inheritParent" : false
}

Also, I'd like to know if the preferred file name should be .cflintrc or .cflintrc.json. I realize it's common to leave off the extension, but I am not sure why. I think having the extension makes it more clear what the format is and makes it easier for editors to assist.

TheRealAgentK commented 7 years ago

I think the filename for a schema might as well contain the extension.

TheRealAgentK commented 7 years ago

How about /src/main/resources/schemas to store all those schemas?

KamasamaK commented 7 years ago

@TheRealAgentK Sorry I wasn't clear on my question above. I think the schema should have the appropriate extension as well, but the question was meant to be about file name for the configuration file itself. That is the file that people will actually be using, and many editors will base the language selected on the extension.

TheRealAgentK commented 7 years ago

Ah, ok. The actual filename should for now maybe stay with .cflintrc without extension. We're changing and breaking so many things at the moment already... maybe for 2.0?

KamasamaK commented 7 years ago

@TheRealAgentK From what I can tell, the way that it's currently implemented, it will work with or without the extension, so there won't be any "breaking" involved -- see here and here. I was more wondering what you guys would consider the standard naming as I am writing a plugin that will have the option of creating an empty/default config file.

Once I get a clear understanding of the config format, I will start testing with it. It's just that I noticed for now that the editor was unable to provide any assistance or syntax highlighting without manually selecting the language each time. I can probably do something about that within that editor, and even better with a schema, but a more general solution would be nice.

TheRealAgentK commented 7 years ago

Ehhm, good question. My gut feeling says still .cflintrc - @ryaneberly and others?

ryaneberly commented 7 years ago

👍 on the .cflintrc. We could easily support either, but a lesson learned for me in this process is to support (just) 1 thing well.

TheRealAgentK commented 7 years ago

ALL THE EMOJIS! :)

KamasamaK commented 7 years ago

Ok, so with that out of the way, let's dig into the the configuration content. I am fine with making the schema if I can simply get the information I need and be able to test it. Please verify that the content I pasted above would be the proper base template now. Also, please verify that the file we are talking about corresponds to com.cflint.config.CFLintConfig. If that is the format, I would question the usefulness of having excludes be anything but a list of codes. Is there a benefit to having messageText or severity if they're just going to be excluded anyway?

ryaneberly commented 7 years ago

@KamasamaK, your template above looks correct to me. There is no benefit to including messageText or severity, the filters are driven purely off of the code as of now.

KamasamaK commented 7 years ago

@ryaneberly Thanks for the clarification. No rush on changing anything, but I wanted to clarify if there was something I was missing.

KamasamaK commented 7 years ago

I've put a schema in /src/main/resources/schemas/ in PR #377

TheRealAgentK commented 7 years ago

Now that the schemas are there, should we close this ticket?

KamasamaK commented 7 years ago

I created the schema the best I could with what I could find. There are probably improvements to be made to things like descriptions, but it's a good starting point. Any big changes to the schema would be in 2.0 and better tracked in #355.

cybersonic commented 6 years ago

Does the main README.md actually reflect these changes in 1.3? I am getting various warnings and errors about a config file that worked for 1.2.3. So I guess the .cflintrc file has changed format a bit but to what?

KamasamaK commented 6 years ago

@cybersonic This issue was just about creating the schema, which can be found here. It is also documented a bit in the README.md here. The only change in 1.3 was the addition of parameters. If you are having warnings and errors for your config file, please create a separate issue.

cybersonic commented 6 years ago

I shall do.

Will try to replicate now.

On 3 May 2018, at 16:02, KamasamaK notifications@github.com wrote:

@cybersonic https://github.com/cybersonic This issue was just about creating the schema, which can be found here https://github.com/cflint/CFLint/blob/master/src/main/resources/schemas/.cflintrc.schema.json. It is also documented a bit in the README.md here https://github.com/cflint/CFLint#folder-based-configuration. The only change in 1.3 was the addition of parameters. If you are having warnings and errors for your config file, please create a separate issue.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cflint/CFLint/issues/352#issuecomment-386326759, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJDHE4Zz3W_t_vG2M3HHvd9jZ5Seq0Lks5tuxwQgaJpZM4OZLU1.