-
Notifications
You must be signed in to change notification settings - Fork 7
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
Rename refactoring: keyword fields #444
Rename refactoring: keyword fields #444
Conversation
…ename-refactoring/adt-keyword-fields
ff42902
to
d2ce994
Compare
…ename-refactoring/adt-keyword-fields
rascal-lsp/src/main/rascal/lang/rascal/tests/rename/TestUtils.rsc
Outdated
Show resolved
Hide resolved
rascal-lsp/src/main/rascal/lang/rascal/lsp/refactor/WorkspaceInfo.rsc
Outdated
Show resolved
Hide resolved
rascal-lsp/src/main/rascal/lang/rascal/lsp/refactor/WorkspaceInfo.rsc
Outdated
Show resolved
Hide resolved
// 'y = x.foo; | ||
// ", decls = "data D = d(int foo) | d(int foo, int baz);" | ||
// , cursorAtOldNameOccurrence = 1); | ||
test bool constructorKeywordField() = testRenameOccurrences({0, 1, 2}, " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand I am too late to the party, but what is the rationale not to use tests that show the source code before and after renaming?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test framework only considers occurrences of oldName
(by default, that's 'foo'). For every returned document edit, it checks if it matches one of the expected occurrences. Occurrences are simply indices. If the renaming return an edit that does not refer to a location where oldName
occurs, or when it renames an occurrences of oldName
that is not in the expected list of indices, the test fails.
Additionally, the test framework applies the returned edits to the test workspace (thus obtaining the actual renamed files) and runs the type checker on them.
We could have written the source code as an expectation. This is probably more readable but a lot more to write. Additionally, we still would need to specify from cursor(s) to rename, which, as @rodinaarssen noted, is the same set of indices as the expected renamed occurrences. All in all, this notation is a lot more concise and requires less modifications when extending existing tests.
@@ -165,3 +165,13 @@ test bool dataTypesInIIModuleStructure() = testRenameOccurrences({ | |||
'bool func(Foo foo) = foo == f();", {0}), byText("RightUser", "import RightExtender; | |||
'bool func(Foo foo) = foo == g();", {}) | |||
}, oldName = "Foo", newName = "Bar"); | |||
|
|||
test bool sameIdRoleOnly() = testRenameOccurrences({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain how these tests work? What exactly is being renamed and how is the result being tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I answered this while giving context here. Please let me know if anything is still unclear!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, these are multi-module. They specifiy the contents and to-be-renamed per module. Otherwise, this uses the same machinery as the single-module tests.
Quality Gate passedIssues Measures |
3050e2b
into
feature/rename-refactoring/cross-module
Implements renaming of keyword fields and unifies this with the collection field machinery.