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

Fixes 4349...take two #4520

Merged
merged 3 commits into from Nov 16, 2018
Merged

Conversation

BZngr
Copy link
Contributor

@BZngr BZngr commented Nov 14, 2018

Resolves #4349. Removes Enums/UDTs rename conflict detection false-positive.
Added a failing repro test for last reported false-positive and some tweaks to the VBA input code for prior #4349 tests.

@bclothier bclothier added the PR-Status: Review Requested No more commits, PR is ready for the eyes that need to see it. label Nov 14, 2018
[Test]
[Category("Refactorings")]
[Category("Rename")]
public void RenameRefactoring_DoesNotWarnForMember_Issue4349()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a question to @rubberduck-vba/contributors -- do we have any consensus on whether the issue # should be embedded in the test? I get the benefit but I would normally put them in a comment rather than in the name.

Copy link
Contributor

Choose a reason for hiding this comment

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

I generally prefer a hyperlink to the issue in a comment above the test if for no other reason that usability.

Choose a reason for hiding this comment

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

I vote keeping issues and their URL links in comments as I believe improves/strengthens our Test driven culture for RD with better evidence tracking for regression anaylsis if need be.
Are there any limitation of how long the URL be kept in comments similar to 255 characters if so, any shortening URL thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm in agreement with using a hyperlink to the issue in a comment...and have updated the tests accordingly.

@bclothier bclothier merged commit c5c9083 into rubberduck-vba:next Nov 16, 2018
@BZngr BZngr deleted the 4349_RenameClash_2 branch June 4, 2020 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Status: Review Requested No more commits, PR is ready for the eyes that need to see it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants