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

Implement phpactor-action-collection #39

Merged
merged 1 commit into from
Aug 6, 2018

Conversation

kermorgant
Copy link
Contributor

resolves #36

@kermorgant kermorgant changed the title Implement phpactor-action-collection WIP : Implement phpactor-action-collection Jul 29, 2018
phpactor.el Outdated
"WIP: Executes a collection of actions."
(mapc
(lambda (action)
(apply #'phpactor-action-dispatch (phpactor--rpc (plist-get action :name) (plist-get action :parameters)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, this does not work for the close_file action. close_file should not trigger a rpc call, but should only be handled within emacs

phpactor.el Outdated
@@ -281,7 +281,7 @@

(cl-defun phpactor-action-close-file (&key path)
"Close file from Phpactor."
(message "close-file action"))
(kill-buffer (find-file-noselect path)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does the job but as path is the path before renaming, it sends a warning

"File path no longer exists!"

@kermorgant kermorgant force-pushed the phpactor-action-collection branch from aeea96d to aca70a1 Compare July 29, 2018 13:17
@kermorgant kermorgant changed the title WIP : Implement phpactor-action-collection Implement phpactor-action-collection Jul 29, 2018
@kermorgant kermorgant force-pushed the phpactor-action-collection branch 2 times, most recently from 154f635 to bfbae17 Compare July 29, 2018 13:20
@kermorgant
Copy link
Contributor Author

I don't see what to change/add for now, but this needs to be tested a bit.

My plan is to use that for some days and see how it behaves.

Any input welcome :-)

@kermorgant kermorgant force-pushed the phpactor-action-collection branch from bfbae17 to c102f64 Compare July 30, 2018 10:25
@kermorgant
Copy link
Contributor Author

2 small issues to fix

  • moving a class can take some time, and UI is frozen during this operation. Not sure what would be the appropriate solution to that.

  • you can answer yes to confirm the operation, but if you answer no, it is ignored and the question remains.

phpactor.el Outdated
@@ -177,6 +177,7 @@
(let ((use-dialog-box nil)
(type (if type (intern type) value-type)))
(cl-case type
(confirm (yes-or-no-p label))
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: You can use y-or-n-p instead of yes-or-no-p. There is no clear standard between these, but in general it is preferable to use yes-or-no-p for "serious" choices.

https://www.emacswiki.org/emacs/YesOrNoP

@kermorgant
Copy link
Contributor Author

The issue with answering no is that the action is just looping on itself from here

(cl-defun phpactor-action-input-callback (&key callback inputs)
  "Require `INPUTS' and dispatch `CALLBACK'."
  (let* ((input-vars (phpactor-action--collect-inputs inputs))
         (parameters (phpactor-action--fill-vars (plist-get callback :parameters) input-vars)))
    (message "%s" callback)
    (apply #'phpactor-action-dispatch (phpactor--rpc (plist-get callback :action) parameters))))

confirmed stays at value nil which is the same as the initial value, and which makes the confirmation question to be asked again.

(:dest_path /path/newfile.php" :confirmed nil :source_path "/path/origfile.php")

phpactor.el Outdated
@@ -177,7 +177,8 @@
(let ((use-dialog-box nil)
(type (if type (intern type) value-type)))
(cl-case type
(confirm (y-or-n-p label))
(confirm (if (y-or-n-p label) t
(error "Action cancelled")))
Copy link
Contributor Author

@kermorgant kermorgant Aug 5, 2018

Choose a reason for hiding this comment

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

not sure using error is the most appropriate thing to do, but at least, it stops the endless confirmation loop

@zonuexe
Copy link
Member

zonuexe commented Aug 6, 2018

LGTM

@kermorgant
Copy link
Contributor Author

thanks @zonuexe
well, if it looks good to you, I think it's time for a merge then :-)

phpactor sometimes need to run a sequence of actions, which this
commit adds support for. In order to support class renaming,
phpactor-action-close-file is also added.

resolves emacs-php#36 (class renaming)
@kermorgant kermorgant force-pushed the phpactor-action-collection branch from 3660fc7 to c2f4c3d Compare August 6, 2018 14:12
@kermorgant kermorgant merged commit 09dba9c into emacs-php:master Aug 6, 2018
@zonuexe
Copy link
Member

zonuexe commented Aug 6, 2018

👍

This was referenced Aug 8, 2018
@kermorgant kermorgant deleted the phpactor-action-collection branch September 16, 2018 11:57
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.

phpactor-move-class does not work
2 participants