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 Eldev #669

Merged
merged 9 commits into from
Nov 5, 2023
Merged

Use Eldev #669

merged 9 commits into from
Nov 5, 2023

Conversation

p4v4n
Copy link
Contributor

@p4v4n p4v4n commented Nov 4, 2023

@vemv
Copy link
Member

vemv commented Nov 4, 2023

Awesome! I take it that you're following the example from clj-refactor or cider?

@p4v4n
Copy link
Contributor Author

p4v4n commented Nov 4, 2023

Awesome! I take it that you're following the example from clj-refactor or cider?

Yes. Mostly copied from both the projects with some minor changes.

@@ -46,7 +46,7 @@
(bb-edn-src (expand-file-name "src" temp-dir)))
(write-region "{}" nil bb-edn)
(make-directory bb-edn-src)
(expect (clojure-project-dir bb-edn-src)
(expect (expand-file-name (clojure-project-dir bb-edn-src))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why this is needed. This test was failing only on windows.
I don't have a windows machine to reproduce/fix it properly.

@@ -3129,7 +3129,7 @@ Assumes cursor is at beginning of function."
"Add an arity to a function.

Assumes cursor is at beginning of function."
(let ((beg-line (what-line))
(let ((beg-line (line-number-at-pos))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this change 3 tests for clojure-add-arity were failing with an additional \n in the generated output. Only happens when running tests with eldev.(Might be because what-line is an interactive fn with side effects)

Copy link
Member

Choose a reason for hiding this comment

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

Sounding good. Please confirm that you've run this modified version locally

Copy link
Contributor Author

@p4v4n p4v4n Nov 5, 2023

Choose a reason for hiding this comment

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

I have run it locally and it works fine.
Locally both the versions work but not using what-line is still better.

Copy link
Member

@vemv vemv left a comment

Choose a reason for hiding this comment

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

LGTM - much appreciated work 🙌

- test-ubuntu-emacs-29
- test-ubuntu-emacs-master
- test-windows-emacs-latest
- test-macos-emacs-latest
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- test-macos-emacs-latest
- test-macos-emacs-latest:
requires:
- test-ubuntu-emacs-29

macOS builds are little more precious

@@ -3129,7 +3129,7 @@ Assumes cursor is at beginning of function."
"Add an arity to a function.

Assumes cursor is at beginning of function."
(let ((beg-line (what-line))
(let ((beg-line (line-number-at-pos))
Copy link
Member

Choose a reason for hiding this comment

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

Sounding good. Please confirm that you've run this modified version locally

@@ -3141,7 +3141,7 @@ Assumes cursor is at beginning of function."
(insert "[")
(save-excursion (insert "])\n(")))
((looking-back "\\[" 1) ;; single-arity fn
(let* ((same-line (string= beg-line (what-line)))
(let* ((same-line (= beg-line (line-number-at-pos)))
Copy link
Member

Choose a reason for hiding this comment

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

Please confirm that you've run this modified version locally

@vemv vemv merged commit 481ca48 into clojure-emacs:master Nov 5, 2023
7 checks passed
@vemv
Copy link
Member

vemv commented Nov 5, 2023

Awesome. If you'd wish to do more contributions for clojure-mode and friends, don't hesitate to ping us in our many GH issues, or #cider.

@p4v4n p4v4n deleted the use-eldev branch November 5, 2023 13:19
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