Fix namespace auto open if parsed input is empty #5040
Conversation
vasily-kirichenko
commented
May 30, 2018
vasily-kirichenko
commented
| | _ -> None | ||
| findBestPositionToInsertOpenDeclaration modules scope point entity | ||
| | _ -> | ||
| // we failed to find insertion point because ast is empty for some reason, return top left point in this case |
There was a problem hiding this comment.
Do we want to do this anytime there is an empty ast?
What if we have a comment at the top of the file with open declarations below it? Would that mean the new open declaration will go above the comment?
There was a problem hiding this comment.
Also, could we find out why the ast is empty?
There was a problem hiding this comment.
What if we have a comment at the top of the file with open declarations below it? Would that mean the new open declaration will go above the comment?
Yes. Or maybe on top of the comment. Why?
Also, could we find out why the ast is empty?
Why? We cannot find a position to insert the open statement, that's enough. Or you suggest to fix the parser?..
There was a problem hiding this comment.
I looked at this, and it seems like the right fix to me, tryFindNearestPointAndModules is looking for the nearest open so that it can insert the new one below it, and finds none.
In findNearestPointToInsertOpenDeclaration when no open was found it quite reasonably says. Oh well, I couldn't find anywhere to insert the open, I guess I will use the top of the file.
It might be interesting to skip over leading comments, but that is a new feature and is not the intent of this PR.
There was a problem hiding this comment.
Skipping over comments would be not be trivial as we don't retain comments in the AST.
The reasoning why I mentioned it was because I thought the AST was giving us nothing even if we had open declarations, and we would just shove a open declaration at the top of the others, potentially skipping a comment if there was one there. However, I misinterpreted this. The case he is fixing is when there are no open declarations, which makes this fix absolutely fine.
There was a problem hiding this comment.
@vasily-kirichenko Thank you for your continued work.
In the future, I recommend being more descriptive in the PRs rather than only showing a GIF. A GIF is great, but we need context.
| | _ -> None | ||
| findBestPositionToInsertOpenDeclaration modules scope point entity | ||
| | _ -> | ||
| // we failed to find insertion point because ast is empty for some reason, return top left point in this case |
There was a problem hiding this comment.
I looked at this, and it seems like the right fix to me, tryFindNearestPointAndModules is looking for the nearest open so that it can insert the new one below it, and finds none.
In findNearestPointToInsertOpenDeclaration when no open was found it quite reasonably says. Oh well, I couldn't find anywhere to insert the open, I guess I will use the top of the file.
It might be interesting to skip over leading comments, but that is a new feature and is not the intent of this PR.
There was a problem hiding this comment.
I misinterpreted what this does. Yes, we should put the open at the top of the file if we can't find any open declarations. Judging from the comment, it didn't sound like that; it sounded more broken rather than just not finding any open declarations.
But now that I understand, this is perfectly reasonable.
@vasily-kirichenko Thank you for this fix.
3 participants
