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

Issue 3105 #3113

Closed
wants to merge 2 commits into from
Closed

Issue 3105 #3113

wants to merge 2 commits into from

Conversation

rnveach
Copy link
Member

@rnveach rnveach commented Apr 17, 2016

Issue #3105

First part of the changes to the test.
We now verify that the input file has the same indentation as the CS violation and has the same expected indentations as the CS violation. The test still can't fully verify non-CS violation comments.

Comments now have a new attribute, ioffset (short for indent offset). This is used when the CS violation's current indentation is different than the indentation of the first character found in the line. It's default value is 0. Reasoning why needed.
CS reported violation indent = indent + ioffset.
Note: Some places where we use this with a large value may have to be inspected as possible issues.

Other changes include streamlining test code by removing excess regular expressions, new audit listener to grab the violations, and having a single class hold all the data on the comment's attributes. All the original check and balances still remain.

@rnveach rnveach force-pushed the issue_3105 branch 2 times, most recently from 83203c3 to 3799c41 Compare April 17, 2016 01:13
@romani
Copy link
Member

romani commented Apr 18, 2016

@rnveach ,

I do not like ioffset , but I am ok with it for temporal solution before we fix problem in Check.
Do you want me to merge this PR (I hope such parameter should be removed after bug in Check is fixed.) ? please open issue on this first.

@rnveach
Copy link
Member Author

rnveach commented Apr 18, 2016

@romani Lets merge and use new issue to verify if they will stay or not.
Some ioffset may be removed, but I think others will stay.
Indentation can throw a violation on any node in the line, there is no guarantee that it will always be the first in the line, unless we are going to make this a requirement. indent is the indentation of the first node in the line. If we are to validate it in the violation message, then ioffset must stay for violations that are not referencing the same node as indent.

@romani
Copy link
Member

romani commented Apr 18, 2016

please do issue first.

@rnveach
Copy link
Member Author

rnveach commented Apr 18, 2016

@romani Issue is open.

@romani
Copy link
Member

romani commented Apr 18, 2016

merged as FF

@romani romani closed this Apr 18, 2016
@rnveach rnveach deleted the issue_3105 branch April 18, 2016 16:21
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

Successfully merging this pull request may close these issues.

None yet

2 participants