Fix currentTopLevelFunction function for metadata skipping

[sashton]

What has Changed?

- This PR updates the forwardSexp function for the case where there exists metadata between two id nodes and skipMetadata==true. Previously, this function would move forward through whitespace and metadata, finally landing on an id node. However, in the switch (token.type) ... case 'id' code there would be one final call to this.next(), thereby moving the cursor past the correct s-expr.
- Now, the switch (token.type) ... case 'id' scenario has been updated to differentiate between:
- the initial iteration of the while loop, where the cursor is still on the original id node, and it is correct to move past this original id node
- any subsequent iteration of the loop, where it is not correct to move the cursor past the current id node

Update:

• The original solution of updating fowardSexp caused unintended consequences with other cursor-motion scenarios. I have reverted my changes there, and instead made the change in currentTopLevelFunction.
• This change updates currentTopLevelFunction by checking whether the cursor is on metadata, and, if so, moves forward via fowardSexp until a non-metadata node is found. It is a simpler, more specific solution than the first attempt.

Fixes #1463

My Calva PR Checklist

I have:

• Read How to Contribute.
• Directed this pull request at the dev branch. (Or have specific reasons to target some other branch.)
• Made sure I have changed the PR base branch, so that it is not published. (Sorry for the nagging.)
• Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.
• Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and either tested it there if so, or mentioned it in the PR.
• Added to or updated docs in this branch, if appropriate
• Tested the VSIX built from the PR (so, after you've submitted the PR). You'll find the artifacts by clicking Show all checks in the CI section of the PR page, and then Details on the ci/circleci: build test.
  • Tested the particular change
  • Figured if the change might have some side effects and tested those as well.
  • Smoke tested the extension as such.
• Referenced the issue I am fixing/addressing in a commit message for the pull request.
  • If I am fixing the issue, I have used GitHub's fixes/closes syntax
    - [ ] If I am fixing just part of the issue, I have just referenced it w/o any of the "fixes” keywords.
• Created the issue I am fixing/addressing, if it was not present.

Ping @PEZ, @bpringe

[PEZ]

Thanks!

Some tests are failing now. Do you know how to run the tests locally? Let me know otherwise and I'll show you.

[sashton]

@PEZ I see the failures. I'm investigating. I may need to solve this a different way. From reading the failed tests, I think I had an incorrect mental model of how forwardSexp was designed to operate over metadata and reader tags.

[PEZ]

Those parts are quite messy. We should clean it up some day.

[sashton]

@PEZ I've reverted the original change and instead made the change in currentTopLevelFunction. All tests now pass, including the new one I added to cover the scenario in the bug report.

[PEZ]

I'm having a look and a think about this. You have probably found the right place to do this, but at the same time this function is also very central to a lot of things. And the current tests do pass. I just need to experiment and think a bit. Please don't hesitate to bump this if I go silent for more than a few days.

[PEZ]

Right so I did go silent... But now have had a look and a think. This seems like the right way to solve the problem so I am going to go ahead and merge. Many thanks for helping in improving Calva!