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

Config file usage #362

Closed
KamasamaK opened this issue Jul 20, 2017 · 20 comments
Closed

Config file usage #362

KamasamaK opened this issue Jul 20, 2017 · 20 comments
Assignees
Milestone

Comments

@KamasamaK
Copy link
Collaborator

KamasamaK commented Jul 20, 2017

I was struggling a bit with getting the JSON configuration file to work, and here is what I found.

  1. When/How should this rule config file read? Should it always be looked for when scanning a file or only for a folder?
  2. Is the file type that is looked for during scan the same as the one passed in the command line as configfile?

My take:
I would say that the way it should work is that it should always look for that file when scanning regardless of whether it's a file or folder. Also, I would expect that if you passed a file as the configfile through the command line that it would process like the project config.

This is what I've found looking briefly through the code:
According to com.cflint.config.CFLintConfig, both excludes and includes are lists of com.cflint.config.CFLintPluginInfo.PluginInfoRule.PluginMessage. This is also somewhat supported by the documentation. I've been unable to get it to work this way through the command line. From what I can gather, the reason that didn't work and to address item 2. above is that it looks like the JSON config file is unmarshalled to com.cflint.config.CFLintPluginInfo from the command line, whereas the XML format can be either.

Correct me on anything I got wrong, but let's discuss how it does and should work. I think we need to address all of these before deprecating the XML formatted config and certainly should make it clear if the command line's config file is different.

@ryaneberly
Copy link
Contributor

@KamasamaK,
1. If you mean the .cflintrc file, I think... when you pass a file parameter, cflint should look for a .cflintrc in that folder and any parent folders - the question would be when to stop? at root? I supposed fixing item 2 would negate the need for looking up the tree?

  1. your analysis is correct. That is inconsistent. I can make the json config support either for 1.2.0 -- does that work for you?

backstory:
Originally the -configfile was intended for a complete replacement of the CFLint base configuration. (cflint.definition.json) I'm not sure that use case is even valid. Fixing 2 would be easier if the json config assumes the .cflintrc form.

@KamasamaK
Copy link
Collaborator Author

KamasamaK commented Jul 22, 2017

@ryaneberly
Yes, the .cflintrc file. It seems we agree then that it should look in that folder and any parent folders. Typically, yes, the config file search would traverse the hierarchy until the root. I think fixing item 2 would negate the need to look for a config file at all if it's passed, but if it isn't passed then it should always look for one. I bring this up is because it is not currently checking for a config file (.cflintrc) when you are passing a single file. It appears that the issue is the check if (folderOrFile.isDirectory()) which won't be true if you're only passing a single file.

Adding the JSON config support for the command line for 1.2.0 would be great!

ryaneberly added a commit that referenced this issue Jul 22, 2017
@ryaneberly
Copy link
Contributor

The configuration reads .cflintrc in the current folder and it's parent folders relative to the -folder argument now.

ryaneberly added a commit that referenced this issue Jul 22, 2017
@KamasamaK
Copy link
Collaborator Author

@ryaneberly Great. Is that also relative to the -file argument?

@ryaneberly
Copy link
Contributor

ryaneberly commented Jul 22, 2017 via email

@KamasamaK
Copy link
Collaborator Author

@ryaneberly Thanks! I have confirmed that passing the JSON config file into the command line works, so consider 2. fixed. In my test using -file , I found that it now works if .cflintrc is in the current directory, but not if it's in the parent.

@ryaneberly ryaneberly added this to the 1.2.0 milestone Jul 25, 2017
@ryaneberly
Copy link
Contributor

@KamasamaK,
There is no difference between -folder and -file, they are aliases. I don't think you will experience different .cflintrc behaviour between them. I tested them both with a .cflintrc file in a parent folder and it worked as expected.

@KamasamaK
Copy link
Collaborator Author

@ryaneberly I am seeing the same functionality between -file and -folder. The issue I was encountering was that I was unable to get the .cflintrc file in a parent directory to be recognized. After further testing, I found that the issue only occurs when using the relative path to the .cfm file. With the absolute path, it works fine.

@ryaneberly
Copy link
Contributor

Hmm.. that helps. is the .cflintrc above the relative path ? That might be an ok functionality

@KamasamaK
Copy link
Collaborator Author

KamasamaK commented Jul 25, 2017

@ryaneberly Yes, it is.

To makes things simple, I placed CFLint-1.2.0-SNAPSHOT-all.jar and test.cfm in C:\CFLint\Child. From the command line (Windows PowerShell) in that same directory, when I run java -jar .\CFLint-1.2.0-SNAPSHOT-all.jar -file .\test.cfm -text with .cflintrc in the same directory, it works fine. If .cflintrc is in the parent (C:\CFLint) then it does not find it. If I instead run java -jar .\CFLint-1.2.0-SNAPSHOT-all.jar -file C:\CFLint\Child\test.cfm -text then it works fine for both same directory and parent.

To be clear, now that I know this only affects relative path usage, this no longer falls into the functionality that I requested for 1.2.0. It would obviously be nice to have, but not a blocker in my opinion.

@KamasamaK
Copy link
Collaborator Author

KamasamaK commented Jul 27, 2017

After some additional testing, I've noticed that the .cflintrc file is also being ignored for paths (even absolute) that are passed into -stdin.

@ryaneberly
Copy link
Contributor

Hmm... Are you piping a directory (not a file) to stdin? I wouldn't
expect that to work

@KamasamaK
Copy link
Collaborator Author

I'm piping the contents of a file and passing the path to that file. Here's an example of what's executed through the command line: java -jar C:\Users\mtbro_000\CFLint\CFLint-1.2.0-all.jar -stdin C:\CFLint\Child\test.cfm -q -e -json -stdout. The stdin is being piped from a text editor.

I was expecting it to check for a config file based on the path passed. For now, I will continue to have the extension handle checking and passing a -configfile.

@ryaneberly
Copy link
Contributor

Interesting. I agree, the could/should work. #373 added.

@ryaneberly
Copy link
Contributor

Can we close this one?

@KamasamaK
Copy link
Collaborator Author

Since stdin has its own issue, the only one mentioned here that I believe is still unresolved is checking for .cflintrc from the command line when passed a relative path. Is that something you think should work?

@ryaneberly
Copy link
Contributor

ryaneberly commented Jul 29, 2017 via email

@KamasamaK
Copy link
Collaborator Author

I had tested and confirmed those first two. That last one I hadn't tested, but I guess that makes sense.

@TheRealAgentK
Copy link
Collaborator

@ryaneberly to follow up what happened to this fix :)

@ryaneberly
Copy link
Contributor

Found the problem. java File.getParent() will return null for relative paths. Changed to File.getAbsoluteFile().getParent() will return the parent folder regardless.

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

No branches or pull requests

3 participants