Removal of s.el as a dependency #372

dotemacs · 2017-04-27T10:23:11Z

This PR based on the this issue #293 "Removal of s.el as a dependency".

The dependency on s.el has now been removed.

Please provide your feedback about the work I've done so far.

Regarding the PR etiquette below, I've crossed out the sections that I don't think are applicable.

Before submitting a PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

The commits are consistent with our contribution guidelines
You've added tests (if possible) to cover your change(s)
The new code is not generating byte compile warnings (run cask exec emacs -batch -Q -L . -eval "(progn (setq byte-compile-error-on-warn t) (batch-byte-compile))" clj-refactor.el)
All tests are passing (run ./run-tests.sh)
~~- [ ] You've updated the changelog (if adding/changing user-visible functionality)~~
~~- [ ] You've updated the readme (if adding/changing user-visible functionality)~~

Thanks!

As per clojure-emacs#293 replacing s.el with subr-x.el All the `thread-last' got unwound due to the re-write as s.el's `s-join' expected separator first then the strings. Whereas subr-x.el's `string-join' expects strings and then the separator.

As per clojure-emacs#293 replacing s.el with subr-x.el

As per clojure-emacs#293 replacing s.el functions with the ones that come as default with Emacs

When using cljr--project-dir with the stock Emacs function string-remove-prefix, like this: (string-remove-prefix (cljr--project-dir) path) instead of s.el's s-chop-prefix: (s-chop-prefix (cljr--project-dir) path) it'll bomb out because cljr--project-dir will return nil, like it does when running the tests. But if it returns a blank string, then there is no issue.

As part of this issue: clojure-emacs#293

As part of the removal of s.el as per clojure-emacs#293 using functions that come as part of Emacs

As per clojure-emacs#293 replacing s.el functions with the ones that come as default with Emacs

dotemacs · 2017-04-27T13:50:19Z

clj-refactor.el

+    (while (not (eobp))
+      (subword-forward)
+      (insert " "))
+    (mapconcat 'identity (split-string (downcase (buffer-string))) "-")))


What do you think of this?

I'm a bit undecided.

In one way it only uses functions that come as part of Emacs and relies on subword mode. But a bit unsure about it... suggestions welcome.

Looks OK to me. If it works as it should, I don't see a huge point in bikeshedding implementation details we can change at will :)

Looks OK to me.

Cool, glad that you approve :)

As part of the drive to replace the dependency on the functions from s.el replacing s-dashed-words with function that relies only on functions that come as part of Emacs release.

thread-last seems to be doing the same thing and is part of Emacs' subr-x.el whereas s-with is part of s.el

As part of the attempt to remove the dependency on s.el, replaced s-slice-at with a simpler/cruder function.

Removed the unnecessary parenthesis within thread-last macros

Replacing it with split-string, but with the parameter taken from s-lines

As per the issue clojure-emacs#293 removed s.el so that there is one less dependency for this mode that isn't part of the standard Emacs release.

dotemacs · 2017-04-27T21:43:56Z

I've removed all the functions from s.el and used stock ones from Emacs.

I've updated the PR description with some comments, please read that first as there are a few outstanding issues that I require your feedback on.

Then please review the code.

Thank you :)

benedekfazekas · 2017-04-30T10:41:53Z

to address your comments above in bold:

contributing guidelines here: https://github.com/clojure-emacs/clj-refactor.el/blob/master/.github/CONTRIBUTING.md perhaps the link is wrong in the PR template?!

the README also has the (correct) link

tests: this is internal refactoring i guess we need to rely on our existing test set to catch regressions... don't think you need to add new unit like tests for this.

benedekfazekas · 2017-04-30T11:28:05Z

clj-refactor.el

@@ -573,7 +573,7 @@ list of (fn args) to pass to `apply''"

 (defun cljr--whitespacep (s)
  "True if S contains only whitespace."
-  (s-blank? (string-trim s)))


essential difference between s-blank? and string-blank-p that former is null safe latter is not. I don't think that causes any problem in this commit but you might want to dblcheck that..

Good point, added the null safety with 36a886b

benedekfazekas · 2017-04-30T11:46:36Z

clj-refactor.el

+			    (locate-dominating-file default-directory "build.boot")))
+			 (ignore-errors (file-truename
+					 (locate-dominating-file default-directory "pom.xml"))))))
+    (if (null project-dir)


perhaps rather (or project-dir "")?

or in fact you can get rid of the let you introduced and just stick an empty string at the end of the original or expression as default?

Good point.

Changed it as per your comment.

I was more on the 'drive' to complete the whole task, so I've missed the obvious stuff like this. Thanks

benedekfazekas · 2017-04-30T11:48:35Z

clj-refactor.el

@@ -2689,9 +2689,9 @@ str/split => split"
  (let ((name (cljr--normalize-symbol-name symbol)))
    (cond
     ((string-match-p "\\w+\\.\\w+" name)
-      (thread-last name (s-split "\\.") last car cljr--symbol-suffix))
+      (thread-last name (split-string "\\.") last car cljr--symbol-suffix))


this should be thread-first as well, or?

Good catch. Fixed.

Shame that the tests didn't catch this...

tests: indeed

I'd happily add the test for this, but seeing as cljr--symbol-suffix is a non-interactive function and most of the tests are feature tests, should I have a step like this:

When I press "M-: (cljr--symbol-suffix \"java.util.Date\")" Then I should see message "Date"

What are your thoughts?

we decided to go down the behavioural testing route not the unit testing one. which is was a good choice i think. so i suppose just leave it ;)

benedekfazekas · 2017-04-30T12:07:21Z

clj-refactor.el

@@ -2247,15 +2247,15 @@ See: https://github.com/clojure-emacs/clj-refactor.el/wiki/cljr-update-project-d
              (insert name)
            (mc/create-fake-cursor-at-point)))
        (re-search-forward "\\[")
-        (when (s-present? locals)
+	(when (not (or (null locals) (string= "" locals)))


i would possibly rather go for
string-empty-p instead of string= "" for readability and perhaps factor this pred out in its own function? something like cljr--string-present-p?

Created cljr--string-present-p as per your comment...

benedekfazekas · 2017-04-30T12:26:19Z

made couple of comments. hope they make sense. only one of those is a potential bug the rest is more like style etc things.

As part of the extraction of the dependencies on s.el, it was replaced with Emacs native functions, but then it was deemed that their usage was not straightforward enough, hence cljr--string-present-p

benedekfazekas · 2017-05-01T20:13:09Z

clj-refactor.el

-    (if (null project-dir)
-	""
-      project-dir)))
+    (or project-dir "")))


do you need the let at all tho? you can just

(or (ignore-errors...) (ignore-errors...) (ignore-errors...) "")

or am i missing something?

benedekfazekas · 2017-05-01T20:14:52Z

made one more comment. otherwise LGTM. great work!

benedekfazekas · 2017-05-01T20:22:12Z

one more thing: cljr--string-present-p could be used at more places i am guessing. for example line 2264 and 3207

Using it in a few more places

Onliner instead of if statement

It should have been a thread first since the switch from s.el's s-split takes regexp as the first argument and the string as the last. Whereas split-string takes the arguments the other way around.

After removing s-blank? from s.el, just using string-blank-p wasn't null safe. Added the null check on Benedek Fazekas' prompting.

No need for the let clause, removing it

dotemacs · 2017-05-01T22:21:45Z

Reused cljr--string-present-p where suitable... 1641730
Removed let c02c15f & reduced repetition 4c0b23b

See what you think of it now.

Thanks for the feedback

benedekfazekas · 2017-05-02T09:35:58Z

clj-refactor.el

+  (or
+   (thread-last  '("project.clj" "build.boot" "pom.xml")
+     (mapcar 'cljr--locate-project-file)
+     (delete '"")


really like this approach. but i think this line is wrong. cljr--locate-project-file returns nil not empty string if file is not present. should be (delete nil) instead.

Thanks, pushed it as part of c33fc5f

Used mapcar in order to avoid the repetition

benedekfazekas · 2017-05-02T18:15:30Z

great work @dotemacs ta!

dotemacs added 18 commits April 25, 2017 14:51

Replaced s-join with string-join

da35e4b

As per clojure-emacs#293 replacing s.el with subr-x.el All the `thread-last' got unwound due to the re-write as s.el's `s-join' expected separator first then the strings. Whereas subr-x.el's `string-join' expects strings and then the separator.

Replaced s-trim with string-trim

7a06913

As per clojure-emacs#293 replacing s.el with subr-x.el

Replaced s-blank? with string-blank-p

38d081a

As per clojure-emacs#293 replacing s.el with subr-x.el

Replaced s-concat with concat

4b47d41

As per clojure-emacs#293 replacing s.el with subr-x.el

Replace s-starts-with-p with string-prefix-p

46f2a2f

As per clojure-emacs#293 replacing s.el functions with the ones that come as default with Emacs

Replace s-(suf|pre)fix-p with string-(suf|pre)fix-p

5c116f5

As per clojure-emacs#293 replacing s.el functions with the ones that come as default with Emacs

Replaces s-equals-p with string-equal

43e229d

As per clojure-emacs#293 replacing s.el functions with the ones that come as default with Emacs

Replaced s-ends-with-p with string-suffix-p

a380f8e

As per clojure-emacs#293 replacing s.el functions with the ones that come as default with Emacs

Replaced s-replace

435615e

As per clojure-emacs#293 replacing s.el functions with the ones that come as default with Emacs

Replaced s-matches-p

11c1d78

As per clojure-emacs#293 replacing s.el functions with the ones that come as default with Emacs

Remove s-chop-(pre|suf)fix with string-remove-(pre|suf)fix

6d0eaa3

Removed s-(pre|ap)pend

84892b0

Switch to downcase from s-downcase

209bd74

As part of this issue: clojure-emacs#293

Remove s-present?

a7a983a

As part of the removal of s.el as per clojure-emacs#293 using functions that come as part of Emacs

Replaced s-split with split-string

5a3b3cc

As per clojure-emacs#293 replacing s.el functions with the ones that come as default with Emacs

Replaced s-contains-p

d3d3223

As per clojure-emacs#293 replacing s.el functions with the ones that come as default with Emacs

Replace s-match

cb32710

As per clojure-emacs#293 replacing s.el functions with the ones that come as default with Emacs

dotemacs commented Apr 27, 2017

View reviewed changes

dotemacs added 2 commits April 27, 2017 18:38

Replace s-dashed-words

df7c4f1

As part of the drive to replace the dependency on the functions from s.el replacing s-dashed-words with function that relies only on functions that come as part of Emacs release.

Replaced s-with

3504f9e

thread-last seems to be doing the same thing and is part of Emacs' subr-x.el whereas s-with is part of s.el

dotemacs force-pushed the no-more-s branch from a2de6c6 to 3504f9e Compare April 27, 2017 17:39

dotemacs added 4 commits April 27, 2017 21:13

Replace s-slice-at

e0c0ac6

As part of the attempt to remove the dependency on s.el, replaced s-slice-at with a simpler/cruder function.

Less parens

315d517

Removed the unnecessary parenthesis within thread-last macros

Remove s-lines

c1efb08

Replacing it with split-string, but with the parameter taken from s-lines

Removed s.el as a dependency

4a10aea

As per the issue clojure-emacs#293 removed s.el so that there is one less dependency for this mode that isn't part of the standard Emacs release.

dotemacs force-pushed the no-more-s branch from d3a4e3a to 4a10aea Compare April 27, 2017 21:21

dotemacs changed the title ~~No more s.el~~ Removal of s.el as a dependency Apr 27, 2017

benedekfazekas reviewed Apr 30, 2017

View reviewed changes

Created cljr--string-present-p

e812312

As part of the extraction of the dependencies on s.el, it was replaced with Emacs native functions, but then it was deemed that their usage was not straightforward enough, hence cljr--string-present-p

benedekfazekas reviewed May 1, 2017

View reviewed changes

dotemacs added 5 commits May 1, 2017 23:09

Reused cljr--string-present-p

1641730

Using it in a few more places

Cleaning up the intent

80d7fd4

Onliner instead of if statement

Fix for the omission with split-string

01cf772

It should have been a thread first since the switch from s.el's s-split takes regexp as the first argument and the string as the last. Whereas split-string takes the arguments the other way around.

Made cljr--whitespacep null safe

e04912f

After removing s-blank? from s.el, just using string-blank-p wasn't null safe. Added the null check on Benedek Fazekas' prompting.

Let diet

c02c15f

No need for the let clause, removing it

dotemacs force-pushed the no-more-s branch from dd9c342 to 4c0b23b Compare May 1, 2017 22:10

benedekfazekas reviewed May 2, 2017

View reviewed changes

Avoiding repetition

c33fc5f

Used mapcar in order to avoid the repetition

dotemacs force-pushed the no-more-s branch from 00c3e93 to c33fc5f Compare May 2, 2017 12:11

benedekfazekas merged commit b6d0715 into clojure-emacs:master May 2, 2017

dotemacs deleted the no-more-s branch May 2, 2017 18:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removal of s.el as a dependency #372

Removal of s.el as a dependency #372

dotemacs commented Apr 27, 2017 •

edited

Loading

dotemacs Apr 27, 2017

expez Apr 27, 2017 •

edited

Loading

dotemacs Apr 27, 2017

dotemacs commented Apr 27, 2017

benedekfazekas commented Apr 30, 2017

benedekfazekas Apr 30, 2017

dotemacs May 1, 2017

benedekfazekas Apr 30, 2017

dotemacs May 1, 2017

benedekfazekas Apr 30, 2017

dotemacs May 1, 2017

benedekfazekas May 1, 2017

dotemacs May 1, 2017

benedekfazekas May 1, 2017 •

edited

Loading

benedekfazekas Apr 30, 2017

dotemacs May 1, 2017

benedekfazekas commented Apr 30, 2017

benedekfazekas May 1, 2017

benedekfazekas commented May 1, 2017

benedekfazekas commented May 1, 2017

dotemacs commented May 1, 2017

benedekfazekas May 2, 2017

dotemacs May 2, 2017

benedekfazekas commented May 2, 2017

Removal of s.el as a dependency #372

Removal of s.el as a dependency #372

Conversation

dotemacs commented Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

expez Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotemacs commented Apr 27, 2017

benedekfazekas commented Apr 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benedekfazekas May 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benedekfazekas commented Apr 30, 2017

Choose a reason for hiding this comment

benedekfazekas commented May 1, 2017

benedekfazekas commented May 1, 2017

dotemacs commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benedekfazekas commented May 2, 2017

dotemacs commented Apr 27, 2017 •

edited

Loading

expez Apr 27, 2017 •

edited

Loading

benedekfazekas May 1, 2017 •

edited

Loading