More bug fixes from mohbl #78
Conversation
…N errors for ImportEqualsDeclaration nodes by including\ndeclaration names in getFQN
… instead of throw error
…ss with debug logs
|
@mohbl I think I've re-created your PR (cherrypick) with help from the copilot on GitHub. It shows all the tests are passing. I will review the changes. |
fuhrmanator
left a comment
fuhrmanator
left a comment
There was a problem hiding this comment.
@mohbl Unfortunately, since I created the PR, I cannot request changes to it :( But I will merge it when you make the changes.
Essentially, for me to review these commits, I want to see the actual tests that demonstrate where the "duplicate FQN conflicts" were occurring. I did not see any new tests added, so it's not clear to me the cases where those duplicate FQN conflicts were happening. I know you test for cases like "Allow arrow functions in blocks, source files, or call expressions (e.g., D3.js each)" (src/fqn.ts) but can you make a jest test case for that? I know it might not be easy to do this, but it is necessary to see that we don't break that function in the future.
| if (Node.isBlock(parent) || Node.isSourceFile(parent) || Node.isCallExpression(parent)) { | ||
| funcIndex++; | ||
| positionMap.set(arrow.getStart(), funcIndex); | ||
| const { line, column } = sourceFile.getLineAndColumnAtPos(arrow.getStart()); | ||
| // console.log(`[buildMethodPositionMap] Arrow function at ${arrow.getStart()} (line: ${line}, col: ${column}), parent: ${parent.getKindName()}, index: ${funcIndex}`); | ||
| } | ||
| }); | ||
|
|
||
| functionExpressions.forEach(funcExpr => { | ||
| const parent = funcExpr.getParent(); | ||
| if (Node.isBlock(parent) || Node.isSourceFile(parent) || Node.isCallExpression(parent)) { | ||
| funcIndex++; | ||
| positionMap.set(funcExpr.getStart(), funcIndex); | ||
| const { line, column } = sourceFile.getLineAndColumnAtPos(funcExpr.getStart()); | ||
| // console.log(`[buildMethodPositionMap] Function expression at ${funcExpr.getStart()} (line: ${line}, col: ${column}), parent: ${parent.getKindName()}, index: ${funcIndex}`); |
There was a problem hiding this comment.
This code (and much of the other changes) is not tested (I don't see any new tests in the PR). Therefore, it's hard to see what "bug" it's fixing.
Your comments for the commits say things like Fix duplicate FQN errors for nested and top-level modules, yet I don't see any new tests that demonstrate those errors and that they are fixed.
As such, it is hard for me to review this code.
Can you add tests to demonstrate the case(s) that this code "fixes"?
There was a problem hiding this comment.
Good evening, and sorry for the late reply , I was on holiday and traveling this weekend.
I really appreciate your feedback and have taken note of everything. You’re totally right about the missing tests, and I’ll be working over the next few days to add proper test cases that cover the situations this PR is fixing, even the tricky ones.
It might take a bit of effort, but I want to make sure everything is covered properly. I’ll update this PR soon.
Thanks again for the detailed review and your patience, @fuhrmanator 🙏
No description provided.