-
Notifications
You must be signed in to change notification settings - Fork 144
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
Added function sly-import-symbol-at-point to add import-from into thecurrent defpackage form #147
Added function sly-import-symbol-at-point to add import-from into thecurrent defpackage form #147
Conversation
… current defpackage form. This function is bound to C-c i by default, and using it, you can easily simplify your code by moving package prefixes into the defpackage form.
Demo looks good, code also looks good from glancing. I'll try to review it thoroughly tonight or over the weekend at the most |
Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great functionality but code must be cleaned up before merging. After the cleanup a NEWS entry should be added (I can do that).
"This function is applied to a package name before | ||
it is inserted into the defpackage form. | ||
|
||
By default, it is an 'identity, but you may wish |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow completely here. This is used in the import-from
form right? If so, then changing the package name from one convention to another would actually create an error in the code, unless the original package:symbol
designation on which I pressed C-x i
was already erroneous.
So unless, I'm missing something, I think this functionality is orthogonal to this PR and should be discussed separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've used this feature to replace dots with slashes during refactoring of a large system to package-inferred-system
. It was very convenient to automatically replace dots with slashes when creating import-from
form.
Here how I've setup this variable in my config:
(setq sly-import-symbol-package-transform-function
(lambda (package)
(replace-regexp-in-string "\\."
"/"
package)))
contrib/sly-package-fu.el
Outdated
@@ -144,9 +155,12 @@ places the cursor at the start of the DEFPACKAGE form." | |||
(1+ (point)) | |||
(point)))))))))))) | |||
|
|||
(defun sly-export-symbols () | |||
(defun sly-read-symbols () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename this to sly-package-fu--read-symbols
and tweak the docstring to state that it reads a list of symbols following point and is intended to be used inside defpackage forms that list symbols.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
contrib/sly-package-fu.el
Outdated
@@ -144,9 +155,12 @@ places the cursor at the start of the DEFPACKAGE form." | |||
(1+ (point)) | |||
(point)))))))))))) | |||
|
|||
(defun sly-export-symbols () | |||
(defun sly-read-symbols () | |||
"Return a list of symbols inside :export clause of a defpackage." | |||
;; Assumes we're at the beginning of :export |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this assumption still valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this function is generic and can be used inside of any s-exp. So, I've removed the comment.
contrib/sly-package-fu.el
Outdated
@@ -155,20 +169,21 @@ places the cursor at the start of the DEFPACKAGE form." | |||
(save-excursion | |||
(cl-loop for sexp = (read-sexp) while sexp collect sexp)))) | |||
|
|||
(defun sly-package-normalize-name (name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good idea to reuse this code in a function, but please rename it sly-package-fu--normalize-name
to comply with emacs lisp coding practices.
contrib/sly-package-fu.el
Outdated
;; Dealing with import-from | ||
;; | ||
;; MELPA version: (buffer-substring-no-properties point1 pount2) | ||
;; ~/.emacs.d/elpa/sly-20171111.1604/contrib/sly-package-fu.el |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't understand these two lines of comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I've removed this junk :)
contrib/sly-package-fu.el
Outdated
|
||
|
||
(defun sly-import-symbol-from (symbol) | ||
"Accepts " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This docstring could see some work. :-) Explain in one sentence less than 70chars what the function does. Like "Inserts SYMBOL at the correct :import-from sexp".
contrib/sly-package-fu.el
Outdated
|
||
;; We only process symbols in fully qualified form like | ||
;; weblocks/request:get-parameter | ||
(when package |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps its useful, instead of when package
to issue an error
or an user-error
. Like
(unless package
(user-error "Can't guess the package designator of %s" symbol))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
contrib/sly-package-fu.el
Outdated
;; If symbol is not imported yet, then just | ||
;; add it to the end | ||
;; TODO: rename function | ||
(sly-insert-export simple-symbol))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, rename the function to sly-package-fu--insert-symbol
and update all its users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
;; Now, remove symbol at the point from the buffer | ||
(delete-region (point) | ||
(+ (point) | ||
(length symbol-name))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its clearer and more readable to use the left-bound
and right-bound
variables established earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, no, this does not work, because when we insert :import-from
form, left-bound
and right-bound
become obsolete.
So, I've returned back (point)
usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then use markers instead.
sly.el
Outdated
@@ -7077,10 +7077,10 @@ The result is unspecified if there isn't a symbol under the point." | |||
"Return the bounds of the symbol around point. | |||
The returned bounds are either nil or non-empty." | |||
(let ((bounds (bounds-of-thing-at-point 'sly-symbol))) | |||
(if (and bounds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is merely cosmetic if I understand correctly. Leave it out of this PR (if you want, submit it in another one)...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returned if
.
Hi @svetlyak40wt, It's been a while since you proposed this. Do you agree with the proposed changes? I like this feature and think it would be quite useful, but I need to maintain some coding standards for maintainability. |
@joaotavora I'm agree and will make all fixes soon. Just too busy. |
@svetlyak40wt ping? If you prefer, I can make the fixes myself (but I'll basically be rewriting your contribution) |
@joaotavora will try to fix everything within few days. |
… and given detailed docstring.
…e-fu--create-new-import-from.
…-update-import-from-form and given proper docstring.
…ackage designator.
@joaotavora please, take a look. I've made the requested changes. |
Looks much better, just a couple of nitpicks left, which I can fix if you can't find the time. Regarding the transformation function, I'll try to understand what package-inferred-system is and get back to you. If there variable makes sense, then it probably should get a new name, too. |
Package inferred system, is a system where you may to not include all source files into the my-system.asd file. ASDF will infer all components and their dependencies from defpackage's use and import-from forms. Also, when using package inferred system definition, you use "one package per file", having defpackage at the beginning of each file. And for big projects, package names should repeat a filestructure, like "app/db/models/user". This way, when you are referring to a symbols imported from I've created this pull, when working on large refactoring of the Weblocks. Before refactoring I also have had on package per file, but used dots as delimiters. That is why I decided to add a May be it is good idea to rename this variable into the |
Interesting, after I've removed comment, Travis build failed for two configurations. |
* contrib/sly-package-fu.el (sly-import-symbol-package-transform-function) (sly-package-fu--read-symbols): Tweak docstring. (sly-package-fu--normalize-name): Rename from sly-package-normalize-name. Fix indentation. (sly-package-fu--read-symbols): Fix indentation. (sly-defpackage-exports): Use sly-package-fu--normalize-name. Fix indentation. (sly-package-fu--search-import-from): Use sly-package-fu--normalize-name (sly-package-fu--add-or-update-import-from-form): Tweak docstring. (sly-import-symbol-at-point): Tweak docstring. Simplify.
This makes it more on par with sly-export-symbol at point. Also saves the file if sly-package-fu-save-file is non nil. Also mention this feature in NEWS.md. * NEWS.md: Mention sly-import-symbol-at-point * contrib/sly-package-fu.el (sly-package-fu--add-or-update-import-from-form): Actually import the symbol and save the file according to sly-package-fu-save-file. (sly-package-fu-save-file): New variable. (sly-export-save-file): Obsolete alias for sly-package-fu-save-file. * contrib/slynk-package-fu.lisp (import-symbol-for-emacs): New slyfun.
So I merged this and added a few tweaks and improvements. Most notably, the symbol is now actually imported by CL just as in And finally a big thank you! |
This function is bound to C-c i by default, and using it, you can easily simplify
your code by moving package prefixes into the defpackage form.
Here is a demo:
https://youtu.be/pZ72_hI4Tcw