Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document .cflintrc schema #352

Closed
KamasamaK opened this issue Jul 16, 2017 · 29 comments
Closed

Document .cflintrc schema #352

KamasamaK opened this issue Jul 16, 2017 · 29 comments
Assignees
Labels

Comments

@KamasamaK
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

TheRealAgentK commented Jul 16, 2017

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
Copy link
Collaborator Author

KamasamaK commented Jul 16, 2017

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
Copy link
Collaborator

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
Copy link
Contributor

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 ryaneberly self-assigned this Jul 16, 2017
@ryaneberly
Copy link
Contributor

@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
Copy link
Collaborator Author

KamasamaK commented Jul 16, 2017

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
Copy link
Contributor

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

I like the way you are thinking.

@TheRealAgentK
Copy link
Collaborator

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

ryaneberly added a commit that referenced this issue Jul 18, 2017
@KamasamaK KamasamaK changed the title Document .cflintrc schemas Document .cflintrc schema Jul 18, 2017
@KamasamaK
Copy link
Collaborator Author

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
Copy link
Collaborator

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

@TheRealAgentK
Copy link
Collaborator

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

@KamasamaK
Copy link
Collaborator Author

@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
Copy link
Collaborator

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
Copy link
Collaborator Author

KamasamaK commented Jul 18, 2017

@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
Copy link
Collaborator

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

@ryaneberly
Copy link
Contributor

👍 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
Copy link
Collaborator

ALL THE EMOJIS! :)

@KamasamaK
Copy link
Collaborator Author

KamasamaK commented Jul 20, 2017

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
Copy link
Contributor

@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
Copy link
Collaborator Author

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

@KamasamaK KamasamaK added the task label Jul 25, 2017
@KamasamaK
Copy link
Collaborator Author

KamasamaK commented Jul 28, 2017

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

@ryaneberly ryaneberly assigned KamasamaK and unassigned ryaneberly Jul 30, 2017
@TheRealAgentK
Copy link
Collaborator

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

@KamasamaK
Copy link
Collaborator Author

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
Copy link

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
Copy link
Collaborator Author

@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
Copy link

cybersonic commented May 3, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants