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 cycling privacy, collection type, if/if-not #381

Merged
merged 1 commit into from Jun 4, 2016
Merged

Conversation

benedekfazekas
Copy link
Member

  • The commits are consistent with our contribution guidelines
  • You've added tests (if possible) to cover your change(s). Indentation & font-lock tests are extremely important!
  • All tests are passing (make test)
  • The new code is not generating bytecode or M-x checkdoc warnings
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the readme (if adding/changing user-visible functionality)

Migrate cycle privacy, cycle collection type and cycle if/if-not implementations from clj-refactor.el.

Additionally refactor def-threading-test macro to use it for testing cycling stuff too and fix duplicate test names in clojure-mode-refactor-threading-test

(save-excursion
(while (and
(> (point) 1)
(not (eq (string-to-char "(") (char-after)))
Copy link
Member

@Malabarba Malabarba May 26, 2016

Choose a reason for hiding this comment

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

Replace this line and the 3 below with a single looking-at.

@Malabarba
Copy link
Member

Besides the comments. LGTM.

@expez
Copy link
Member

expez commented May 27, 2016

Btw, I recently used this feature in cljr and it occurred to me that we should remove the cycle to and from lists.

Lists aren't used for literal data in clojure, vectors are, so it makes no sense to cycle to and from this collection type imo.

Furthermore, at least in cljr, the list isn't quoted so that part of the refactoring is broken. It's cycling between code and data now :/

@tanzoniteblack
Copy link
Contributor

@expez I rather like having lists in there, as I use it to cycle collections to to function calls quite often. So we're changing between code and data...but that's actually pretty useful, since code is data in clojure.

@benedekfazekas
Copy link
Member Author

thanks for the feedback @Malabarba @expez @tanzoniteblack

re. cycle collection type I think both @expez and @tanzoniteblack have a point here. I don't see cycling between code and data a big problem either, in certain contexts (reagent for example) even stuff in a vector can be an fn invocation.

so I propose to actually add '() to the list of collection types and also leave () there. let me know what you think.

@expez
Copy link
Member

expez commented May 28, 2016

so I propose to actually add '() to the list of collection types and also leave () there. let me know what you think.

When you have to click C-c C-m cc 4 times to get the collection you want you're not saving any time compared to a regular edit. You're now proposing we add another one, to cycle between, set, map, list, quoted list and vector. I think this will be too much.

I'd even like to narrow it down even more, so it only cycles between vectors and sets. Cycling to a map (from a vector/set) or from a map (to a vector/set) is something I've never done either. Have any of you?

A refactoring isn't supposed to change the semantics of the code, but going from data to code certainly does that. I still think the cycle to () is rare enough that we can provide it as a separate function (with our without a keybinding) or even drop it completely.

@benedekfazekas
Copy link
Member Author

too much typing is a valid concern. so I can rewrite this not to be a cycling thing but rather you type C-c C-r [ for a vector or C-c C-r { for a map. for set and quoted list we can use C-c C-r # and C-c C-r ' respectively.

Cycling to a map (from a vector/set) or from a map (to a vector/set) is something I've never done either. Have any of you?

Unfortunately we don't have a way knowing how this (or any) feature is used out in the wild (special thanks @tanzoniteblack for your feedback!). I do remember cycling at least once to a map from a vector when I realised I wanted to use a different data structure. I have edited the map further after changing it to map from vector but this does not really matter I guess.

A refactoring isn't supposed to change the semantics of the code

strictly speaking this is true. However, we do violate this anyway elsewhere, for example cycling privacy changes the public API of a namespaces which is not really a refactoring either strictly speaking... It is still a useful feature.

@expez
Copy link
Member

expez commented May 29, 2016

I really like you suggestion of using additional keybindings. Those keybindings would be weird for anything else, but are easy to remember in this context. 👍

@benedekfazekas
Copy link
Member Author

done from my side i think. although cycle collection is pretty much redone so comments welcome about it.

@Malabarba
Copy link
Member

I like the idea of switching to a specific collection. @bbatsov what do you think?

(when (and (bobp)
(not (memq (char-after) '(?\{ ?\( ?\[))))
(user-error "Beginning of file reached, collection is not found"))

Copy link
Member

Choose a reason for hiding this comment

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

Remove this blank line, and the two above.

@Malabarba
Copy link
Member

Besides the two new comments, LGTM.

Migrate cycle privacy, cycle collection type and cycle if/if-not
implementations from clj-refactor.el.

Cycle collection type is reworked into convert collection with a
dedicated defun/menu item/keybinding for every collection type. Quoted list
is also added to the supported collection types.

Additionally refactor `def-threading-test` macro to use it for testing
cycling stuff too and fix duplicate test names in
`clojure-mode-refactor-threading-test`
@benedekfazekas
Copy link
Member Author

no rush here guys, but I am not aware of anything left to do here...

@Malabarba
Copy link
Member

Thanks again Benedek!

@Malabarba Malabarba merged commit 2fa46e6 into master Jun 4, 2016
@benedekfazekas benedekfazekas deleted the cycling-things branch June 4, 2016 19:00
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.

None yet

4 participants