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

Use refactor-nrepl 3.3.2 #202

Merged
merged 4 commits into from
Feb 10, 2022
Merged

Conversation

vemv
Copy link
Contributor

@vemv vemv commented Feb 8, 2022

Brief

https://github.com/clojure-emacs/refactor-nrepl/blob/bc7779fc0d25ec025486438ad0f5b0990644a742/CHANGELOG.md#330

Now refactor-nrepl honors .clj-kondo config, as far as ns cleaning goes.

Which for us is useful: it means that f-s also gets to reuse kondo config, which means a DRYer/easier experience for a variety of users.

QA plan

  • Add sample :unused-namespace kondo config (example)
  • Verify that f-s will honor that setting

Author checklist

  • I have QAed the functionality
  • The PR has a reasonably reviewable size and a meaningful commit history
  • I have run the branch formatter and observed no new/significative warnings
  • The build passes
  • I have self-reviewed the PR prior to assignment
  • Additionally, I have code-reviewed iteratively the PR considering the following aspects in isolation:
    • Correctness
    • Robustness (red paths, failure handling etc)
    • Test coverage
    • Spec coverage
    • Documentation
    • Security
    • Performance
    • Breaking API changes
    • Cross-compatibility (Clojure/ClojureScript)

Reviewer checklist

  • I have checked out this branch and reviewed it locally, running it
  • I have QAed the functionality
  • I have reviewed the PR
  • Additionally, I have code-reviewed iteratively the PR considering the following aspects in isolation:
    • Correctness
    • Robustness (red paths, failure handling etc)
    • Test coverage
    • Spec coverage
    • Documentation
    • Security
    • Performance
    • Breaking API changes
    • Cross-compatibility (Clojure/ClojureScript)

@vemv vemv force-pushed the refactor-nrepl-3-3-1 branch from 3da7487 to 4f57611 Compare February 8, 2022 09:30
@vemv vemv marked this pull request as ready for review February 8, 2022 09:31
(when-let [ns-replacement (replaceable-ns-form this filename)]
(println "Cleaning unused imports:" filename)
(write-ns-replacement! filename ns-replacement)))))
(with-memoized-libspec-allowlist
Copy link
Contributor Author

@vemv vemv Feb 8, 2022

Choose a reason for hiding this comment

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

This macro invocation could be a strategy instead, added to the stack, but I prefered to leave something that would survive any possible refactorings (as we foresee that will happen wrt config, strategies and stuff) and clearly work no matter what.

Could be revisited later when f-s' new API becomes an actual thing

Copy link
Member

@thumbnail thumbnail left a comment

Choose a reason for hiding this comment

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

🚀 thanks for this one!

{:style/indent 0}
[& body]
`(do
(require 'refactor-nrepl.ns.libspec-allowlist)
Copy link
Member

Choose a reason for hiding this comment

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

What about requiring-resolve?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not available in every clojure version, so I tend not to use it in tooling projs

Copy link
Member

Choose a reason for hiding this comment

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

F-s ships with clojure 1.10.3, so I think you're safe 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vemv vemv changed the title Use refactor-nrepl 3.3.1 Use refactor-nrepl 3.3.2 Feb 10, 2022
Copy link
Member

@thumbnail thumbnail left a comment

Choose a reason for hiding this comment

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

🚀 awesome! thanks

@thumbnail thumbnail merged commit 6699eb1 into nedap:main Feb 10, 2022
@vemv vemv deleted the refactor-nrepl-3-3-1 branch February 10, 2022 15:09
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.

2 participants