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

Add missing move-form clojure-lsp refactoring #2016

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

svdo
Copy link
Contributor

@svdo svdo commented Jan 10, 2023

What has changed?

Clojure-lsp has a move-form refactoring that doesn't have an equivalent command in Calva yet. This PR adds the refactoring.

Fixes #2015

My Calva PR Checklist

  • 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
  • Tests
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
    • Smoke tested the extension as such.
    • 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.
  • 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.
  • Formatted all JavaScript and TypeScript code that was changed. (use the prettier extension or run npm run prettier-format)
  • Confirmed that there are no linter warnings or errors (use the eslint extension, run npm run eslint before creating your PR, or run npm run eslint-watch to eslint as you go).

Ping @PEZ, @bpringe, @corasaurus-hex, @Cyrik

@svdo
Copy link
Contributor Author

svdo commented Jan 10, 2023

Hi Calva team, can you have a quick look at my first commit on this branch please? I'm wondering if this is all there's to its (to add the refactoring commands on the Calva side), or if I'm missing things. Next, I will compare the list in Clojure-lsp and see if there are more missing refactorings.

Also, it seems to me that extraParamFn: makePromptForInput('Bind to') isn't doing anything. I'd expect it to ask for the name of the binding, but it doesn't. Also for (some?) other commands this doesn't seem to work for me. Can somebody confirm whether this is broken or working as intended for them? Thanks!

Copy link
Collaborator

@PEZ PEZ left a comment

Choose a reason for hiding this comment

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

LGTM so far. There was a thread on the refactorings over at the Clojurians Slack the other day, where some other missing refactors were missing. Don't know if you want to address them at the same time. It's fine with me either way.

@PEZ
Copy link
Collaborator

PEZ commented Jan 10, 2023

And now I saw your questions, @svdo. Will have a look.

@svdo svdo force-pushed the add-missing-lsp-refactorings branch from 6931d3a to 08a45ec Compare January 25, 2023 09:42
@svdo svdo changed the title Add missing clojure-lsp refactorings Add missing move-form clojure-lsp refactoring Jan 25, 2023
@svdo svdo force-pushed the add-missing-lsp-refactorings branch from 08a45ec to 71bf46c Compare January 25, 2023 09:59
@svdo
Copy link
Contributor Author

svdo commented Jan 25, 2023

Today I had some time to look into this again. I discovered that in fact the destructure-keys and restructure-keys refactorings are available through the quick fix menu.

On Slack I found someone mention that move-form was not implemented, so I added that instead.

@svdo svdo marked this pull request as ready for review January 25, 2023 10:08
@bpringe
Copy link
Member

bpringe commented Jan 27, 2023

Also, it seems to me that extraParamFn: makePromptForInput('Bind to') isn't doing anything. I'd expect it to ask for the name of the binding, but it doesn't.

@svdo I tried using the commands for moving a form to the previous let form, for extracting to a new function, and for introducing a let, and they all ask me for input for what to bind it to. I ran these commands from the command palette.

Also for (some?) other commands this doesn't seem to work for me

What do you mean here?

On another note, there are some merge conflicts. Would you mind fixing those?

@svdo
Copy link
Contributor Author

svdo commented Jan 27, 2023

@bpringe This problems I mentioned were a few weeks ago. I'm not sure what was going on, but when I looked at it again this week it seemed to be working fine, so you can ignore those remarks.

Regarding the merge conflict: apparently the file I changed has been deleted. Git is not very smart that way :( I tried making the changes again by merging in dev but Clojure-lsp doesn't seem to work anymore for me after that merge. I'll try to find some time soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants