Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Feb 15, 2016

The Declaration.Selection is the ambiguousidentifier's selection which is always only the first line. That's why this fix uses the procedure's context which spans the entire procedure which is what we probably want here.

retailcoder added a commit that referenced this pull request Feb 16, 2016
@retailcoder retailcoder merged commit dea0f60 into rubberduck-vba:next Feb 16, 2016

public override void Fix()
{
var oldContent = Context.GetText();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the context spans the entire body, this fix is now rewriting the entire thing and deleting all comments in the process, when really all that's needed is adding Public in front of the first token. Seems overkill and potentially irritating, I merged this PR because it fixes the tests, but clearly at least one test is missing for this inspection/fix, since it does not behave at it should - it's way too invasive for what it's doing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it just me or is this fixable by getting comments into the ParseTree as nodes?
That would allow skipping irrelevant content or rather readding it correctly

@retailcoder
Copy link
Member

@Vogel612 indeed, if comments were in the parse tree it wouldn't be a problem, other than still being a bit overkill to rewrite the whole entire procedure only to stick Public in front of the very first token in the whole thing.

@ghost
Copy link
Author

ghost commented Feb 16, 2016

I didn't know that about the comments, sorry! Thanks for fixing it.

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.

2 participants