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

Added more tags and attributes to check in VarScoper #495

Merged
merged 2 commits into from
Dec 20, 2017
Merged

Added more tags and attributes to check in VarScoper #495

merged 2 commits into from
Dec 20, 2017

Conversation

KamasamaK
Copy link
Collaborator

No description provided.

@KamasamaK KamasamaK requested a review from ryaneberly December 19, 2017 21:13
@ryaneberly
Copy link
Contributor

Code looks good. Got a couple failing tests to reconcile:
https://travis-ci.org/cflint/CFLint/builds/318843106?utm_source=github_status&utm_medium=notification

@KamasamaK
Copy link
Collaborator Author

Instead of having those values in com.cflint.plugins.core.VarScoper, do you think it would be better to utilize com.cflint.tools.CFMLTagInfo? From what I can tell, isAssignmentAttribute() should return true for any attribute that should be checked for local scoping.

@ryaneberly ryaneberly added this to the 1.3.0 milestone Dec 20, 2017
@ryaneberly
Copy link
Contributor

@KamasamaK , Yes that is what CFMLTagInfo.isAssignmentAttribute is for. Though it isn't currently in use and would take some testing.

The three failing tests:

TestCFBugs_VarScoper_Names.java
remove: new String[] { "CFStoredProc" },

TestCFBugs_VarScoper_TagAttr.java
remove : retval.add(new String[] { "CFMail", "Query" });

cfloop_index_413.cfc
Accept new result: column 33 is more correct than 2, should really be 37

@KamasamaK
Copy link
Collaborator Author

I had changed the first 2 locally already as it looked easy enough. I was trying to figure out how that integration test was even finding idx when I came across CFMLTagInfo.isAssignmentAttribute but wasn't really sure how it was working.

Regarding the column, yes it does look like the column is more correct now, so however it found it before must not be reporting that correctly. If we take column to be number of characters, then 33 is correct. It would only be 37 if the tab counted as 4 columns. Although the other one in that file for xxx seems to be wrong.

Anyway, the tests are fixed now.

@ryaneberly ryaneberly merged commit b7091f0 into cflint:dev Dec 20, 2017
@ryaneberly
Copy link
Contributor

Thanks @KamasamaK

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

Successfully merging this pull request may close these issues.

2 participants