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

Various smaller issues with output #323

Closed
TheRealAgentK opened this issue Jul 1, 2017 · 16 comments
Closed

Various smaller issues with output #323

TheRealAgentK opened this issue Jul 1, 2017 · 16 comments
Assignees
Milestone

Comments

@TheRealAgentK
Copy link
Collaborator

TheRealAgentK commented Jul 1, 2017

  1. If -verbose is off, CFLint XML should not be output to the console after processing.
  2. CFLint XML should not be output to the console once per output type, i.e. I output XML and HTML and it dumps the underlying XML to the console twice.
  3. CFLint/CFParser tokens are currently output in dev during parsing. That's probably useful info, but only in verbose mode.
  4. JSON and TEXT output don't include the number of files checked and the number of LOC checked information as XML does.
@TheRealAgentK TheRealAgentK self-assigned this Jul 1, 2017
@TheRealAgentK TheRealAgentK added this to the 1.2.0 milestone Jul 1, 2017
ryaneberly added a commit that referenced this issue Jul 1, 2017
3 is fixed.
ryaneberly added a commit that referenced this issue Jul 1, 2017
@ryaneberly
Copy link
Contributor

All except 4 are taken care of. Good catch

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

Cool, will give it a quick test and then then fix number 4 from the list, too.

@TheRealAgentK
Copy link
Collaborator Author

I fixed number 4.

There's an issue in TestFiles though. In JSON, totalfiles and totalsize will always be 0 each. That is because those values get incremented in CFLint.scan(...) which doesn't seem to be used in the TestFiles setup. I'd be happy to accept that those tests will always report 0/0.

I regenerated all the expected test files, and then realised that it has changed the file paths too:

-    "file" : "src\\test\\resources\\com\\cflint\\tests\\BuiltInFunctionChecker\\isDate2.cfm",
+    "file" : "src/test/resources/com/cflint/tests/BuiltInFunctionChecker/isDate2.cfm",

I'm pretty sure that should work across Win/Mac/Linux, but it'd be good if you could test that @ryaneberly .

TheRealAgentK added a commit to TheRealAgentK/CFLint that referenced this issue Jul 2, 2017
TheRealAgentK added a commit to TheRealAgentK/CFLint that referenced this issue Jul 2, 2017
TheRealAgentK added a commit to TheRealAgentK/CFLint that referenced this issue Jul 2, 2017
TheRealAgentK added a commit to TheRealAgentK/CFLint that referenced this issue Jul 2, 2017
@ryaneberly
Copy link
Contributor

ryaneberly commented Jul 2, 2017 via email

@ryaneberly ryaneberly reopened this Jul 25, 2017
@ryaneberly
Copy link
Contributor

I agree with @KamasamaK's comment on #331.

The original JSON format is:
[ {"severity" : "INFO", ...} ]

The current format in dev is:

[ {"severity" : "INFO", ...}, {
  "totalfiles" : 3
}, {
  "totalsize" : 18
}, {
  "code" : "COMPONENT_INVALID_NAME",
  "count" : 1
}, {
  "severity" : "INFO",
  "count" : 1
} ]

I think we need to redefine it as:

{ "issues": [ {"severity" : "INFO", ...} ],
  "summary": {
  "totalfiles" : 3,
  "totalsize" : 18,
  "countByCode" : [
 {
  "code" : "COMPONENT_INVALID_NAME",
  "count" : 1
}],
 "countBySeverity": [ {
  "severity" : "INFO",
  "count" : 1
}]
}

@KamasamaK
Copy link
Collaborator

KamasamaK commented Jul 25, 2017

@ryaneberly I agree with your proposed format. That organization makes much more sense. It's technically a bigger change, but will be easier to re-integrate as will be likely since both changes are breaking. 😀

@TheRealAgentK
Copy link
Collaborator Author

Yes, I agree with that too, much better format - I'll change it tomorrow.

@TheRealAgentK
Copy link
Collaborator Author

Note to self: Change user manual after this change.

TheRealAgentK added a commit that referenced this issue Jul 26, 2017
#323 - Changing JSON structure and changing expected test results
@TheRealAgentK
Copy link
Collaborator Author

I think format is fine now - need someone to have a quick look at three of the tests that behave weird. Let me know what you think, if it's all ok I'll change the user guide.

@ryaneberly
Copy link
Contributor

@TheRealAgentK, Thanks. I'll look tonight

@KamasamaK
Copy link
Collaborator

KamasamaK commented Jul 27, 2017

The structure looks good now. A few comments about the format:

  1. Perhaps we could rename totalsize as totalLines to be more clear on the meaning.
  2. If we are using a camelCase convention, which it seems we are based on other names, then totalfiles should be totalFiles.
  3. The summary object lacks consistent naming with the counts element in the XML format.
  4. Now that it's an object, I think we can safely add some missing XML attributes -- notably, version and timestamp:
{
  "version": "1.2.0",
  "timestamp": "1501170620",
  "issues": [ {"severity" : "INFO", ...}, ... ],
  "summary": { ... }
}

@TheRealAgentK
Copy link
Collaborator Author

TheRealAgentK commented Jul 27, 2017

1/2 - Happy to do that, will name them totalLines and totalFiles in JSON and XML and internally
3 - I won't touch that because counts already existed in CFLint XML since Feb 2016 and I don't want to break existing format in 1.2 - that might happen in 2.0 though
4 - Yes, good call - will do that too.

@KamasamaK
Copy link
Collaborator

@TheRealAgentK Regarding 3, it's up to you guys which name you prefer. Since summary is new to the JSON format as far as I can tell, you could make them consistent by having the JSON use counts instead. I have no strong opinion on which name to use. As for breaking the format, that's already done with the JSON so why stop there? 😄

@ryaneberly
Copy link
Contributor

@KamasamaK
That's fair, but we should still minimize the collateral

@TheRealAgentK
Copy link
Collaborator Author

That's actually a good point.

@TheRealAgentK
Copy link
Collaborator Author

If @KamasamaK doesn't mind I'll rename summary to counts in JSON, then at least XML doesn't break and it's consistent for now.

TheRealAgentK added a commit to TheRealAgentK/CFLint that referenced this issue Jul 28, 2017
…alLines in JSON output). Also changed CFLintStats internal naming. This is OK for CFLint XML changes because it was an attribute added only for 1.2 (no breaking functionality)
TheRealAgentK added a commit to TheRealAgentK/CFLint that referenced this issue Jul 28, 2017
TheRealAgentK added a commit to TheRealAgentK/CFLint that referenced this issue Jul 28, 2017
TheRealAgentK added a commit to TheRealAgentK/CFLint that referenced this issue Jul 28, 2017
TheRealAgentK added a commit to TheRealAgentK/CFLint that referenced this issue Jul 28, 2017
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