Refactor phpactor-update #86

zonuexe · 2019-01-07T13:36:14Z

Changes

Rename phpactor-update to phpactor-install-or-update.
Install Phpactor in ~/.emacs.d/phpactor/ instead of the directory where the package was installed (it is better to decouple those as package updates don't necessarily require an update of phpactor).

kermorgant

Hi @zonuexe I've not had time to test but I've put some questions straight out of my head.

Not sure what is the rationale behind this change : was the previously installation folder for phpactor subject to frequent changes, requiring some useless call to phactor--install-or-upgrade or is it something else ?

kermorgant · 2019-01-07T14:00:54Z

phpactor.el

-    (call-process composer-executable nil (get-buffer-create phpactor--buffer-name) nil "install" "--no-dev")))
+  (let* ((default-directory (phpactor--get-package-directory))
+         (directory (or phpactor--base-directory
+                        phpactor--remote-composer-file-url-dir)))


why (or in which case) would you look into phpactor--remote-composer-file-url-dir ?

kermorgant · 2019-01-07T14:01:24Z

phpactor.el


-(defun phpactor-update ()
+;;;###autoload
+(defun phpactor-install-or-update ()


kermorgant · 2019-01-07T14:03:07Z

phpactor.el

+             for code = (format "copy(%s, %s)"
+                                (php-runtime-quote-string (concat directory file))
+                                (php-runtime-quote-string (concat phpactor-package-directory file)))
+             do (message code)


isn't that message some debug you forgot to remove ?

kermorgant · 2019-01-07T14:10:22Z

phpactor.el

+  :group 'php)
+
+;;;###autoload
+(defcustom phpactor-package-directory


hmm, I'm a bit confused by the package term. for me, package relates to phpactor.el, but it seems we're installing phpactor in this folder.

unless I'm wrong, maybe just phpactor--directory ?

kermorgant · 2019-01-07T14:13:31Z

phpactor.el

+;;; Constants
 (defconst phpactor-command-name "phpactor")
 (defconst phpactor--supported-rpc-version "1.0.0")
+(defconst phpactor--base-directory


just to be sure I understand, this looks like the folder previously returned by phpactor--get-package-directory (ie something like ~/.emacs.d/elpa/phpactor-date). Am I correct ?

kermorgant · 2019-01-07T14:17:33Z

phpactor.el

 (defun phpactor--get-package-directory ()
  "Return the folder where phpactor.el is installed."
-  (file-name-directory(locate-library "phpactor.el")))
+  (or phpactor-package-directory


following my previous comment about this function, maybe it is not needed ?

kermorgant · 2019-01-20T13:14:19Z

Hi @zonuexe

I've just added a commit illustrating my view of the situation. It worked for me but maybe I miss some things you had in mind ?

zonuexe · 2019-02-05T16:18:54Z

@kermorgant I'm sorry, since this commit 2b2695a is not my intended code, I will delete it once in history.

zonuexe · 2019-02-05T17:56:01Z

@kermorgant
I reorganized commits. Please point out again points that you do not know the intention of change.
Thank you.

…rary

Instead of the package's directory, copy the file to the user's .emacs.d directory and install it.

The term "Package" is ambiguous for both Lisp package (ELPA) and PHP package (Composer). Here we used Composer to try to set up Phpactor in a different directory with package.el, which meant Composer package.

Before the change, when phpactor-install-directory was NIL, it falls back to the directory of the Lisp package. However, there is no demand for that function, so we no longer have such an option.

The purpose of this change is to resolve the "package" ambiguity between Lisp and PHP.

zonuexe · 2019-02-08T14:46:42Z

@kermorgant Thank you for reviewing.

I merged this branch into develop. I am not going to merge this commit for a while for a while, so please correct the incomprehensible places and naming incongruity.

kermorgant · 2019-02-08T15:49:09Z

@zonuexe I wonder, did you miss my pending comments about downloading composer.json etc. ?

zonuexe · 2019-02-08T15:52:55Z

@kermorgant Is it 2b2695a or anything else?

kermorgant · 2019-02-08T16:06:28Z

@zonuexe No, I meant those ones, about phpactor--lisp-directory

zonuexe · 2019-02-08T16:13:52Z

@kermorgant I see, the comment is in "pending" state, I can not read it.

kermorgant · 2019-02-06T08:06:27Z

phpactor.el

+  (eval-when-compile
+    (when (and (boundp 'byte-compile-current-file) byte-compile-current-file)
+      (file-name-directory byte-compile-current-file))))
+(defconst phpactor--remote-composer-file-url-dir


@zonuexe I don't understand why we should download composer.json & composer.lock.

Sorry, I started reading + commenting chronologically and now I see your comment from commit e7a7773

Anyway, this elisp code is still obscure to me, and beyond that, the use case that you support

I'm intrigued by the fact we have 2 different alternatives to find the path of phpactor.el, and that we also care for the case when those 2 fail (fallback to download composer.json+composer.lock).

And my misunderstanding starts with the first condition of the if : Under which circumstances would (and byte-compiled-dir (file-directory-p byte-compiled-dir)) fail ?

@kermorgant
Yes, we can not get byte-compiled-dir in the following cases:

Lisp code was directly evaluated on the buffer.

Not byte-compiled Lisp file was loaded

We do not see these symptoms when installing the Lisp package from package.el, but in that case occasionally.

zonuexe · 2019-02-08T16:45:05Z

Why not composer install in the same directory as Lisp?

There are several reasons for this. In Emacs we can run Lisp files in several ways.
Lisp files are not only installed by package.el, they can be introduced by other mechanisms, or the Lisp code can be executed directly from the buffer.
That means there are several cases where directories can not be specified in either byte-compiled-dir or locate-directory.
Or that may not have a directory at all in the first place. I would like to run it even under such circumstances.
In rare cases, it is possible that byte-compiled files are moved to other environments and the paths do not match.

Next, consider the case where it was installed by package.el or byte compiled normally.
In these cases we can assume that we can locate the appropriate composer.json and composer.lock.
However, since package.el contains the version number in the directory name, the user must set up Phpactor each time it is updated.
By install the files in a directory that is not dependent on the directory of the package, users can reduce the number of times to run phpactor-install-or-update frequently.

There are pros and cons in this implementation. By having users composer install in the same location as the Lisp file every time you update, users always get the version of Phpactor recommended by phpactor.el. In the current implementation, the user may continue to use the old Phpactor and cause trouble.

kermorgant · 2019-02-09T16:17:25Z

@zonuexe Thanks for taking the time to explain your reasoning.

I'm fine with the last part, about reducing the number of times to run phpactor-install-or-update frequently. The risk about using an old version of Phpactor is something we should be able to cope with elegantly with Phpactor's RPC versioning.

Then about the first part... First, I should stress once again I'm quite inexperienced with emacs so my view on this is necesserily narrowed. But from a practical perspective, I still can't see why a user would install a package differently than with package.el or cloning the git repo directly.

This is not a big deal per se, I'm fine with this but as I was rewiewing this, I tried to get into the details ;-)
And I guess I insisted a bit on this because I might have an opposite view on such things : I'd personnally be more of a dictator and enforce a standard way of installing this package and by that avoid some corner cases. But again, it's not a problem, it's just to share a bit how I view things.

zonuexe · 2019-02-12T09:14:12Z

@kermorgant

And I guess I insisted a bit on this because I might have an opposite view on such things : I'd personnally be more of a dictator and enforce a standard way of installing this package and by that avoid some corner cases. But again, it's not a problem, it's just to share a bit how I view things.

That idea makes sense. Still do not force all users to install from the package. For example, this implementation could be replaced by a message saying “Please set phpactor-executable or phpactor-lisp-directory.”

The balance between "saving user's labor" and "keeping the code simple" is a difficult decision. In this implementation, I emphasized the user's convenience. When I need to maintain this code several times in the future, I agree to remove it.

zonuexe requested a review from kermorgant January 7, 2019 13:36

kermorgant reviewed Jan 7, 2019

View reviewed changes

zonuexe force-pushed the refactor/phpactor-update branch from 2b2695a to 14c3ea8 Compare February 5, 2019 17:39

zonuexe force-pushed the refactor/phpactor-update branch from 5b3c3d5 to 4dce93b Compare February 7, 2019 14:59

zonuexe force-pushed the develop branch from 598616f to 3a0781f Compare February 8, 2019 14:41

zonuexe and others added 12 commits February 8, 2019 23:41

Add customize variable phpactor-package-directory

b3e98f5

Require php-runtime package for PHP's copy() function

88c735e

Add constant phpactor--base-directory and prioritize it to locate-lib…

4136409

…rary

Rename and refactor phpactor-install-or-update

45bf6cc

Instead of the package's directory, copy the file to the user's .emacs.d directory and install it.

Modify doc comment to clarify intent

72d2584

Rename phpactor-package-directory to phpactor-install-directory

52bff67

The term "Package" is ambiguous for both Lisp package (ELPA) and PHP package (Composer). Here we used Composer to try to set up Phpactor in a different directory with package.el, which meant Composer package.

Expand phpactor--get-package-directory call to a variable

664815c

Before the change, when phpactor-install-directory was NIL, it falls back to the directory of the Lisp package. However, there is no demand for that function, so we no longer have such an option.

Refactory phpactor--base-directory and modify doc comment

7694639

Rename phpactor--lisp-directory and add strict check of file system

7372826

The purpose of this change is to resolve the "package" ambiguity between Lisp and PHP.

Add comment about reason not to use f-join

d96a722

removal of redundant or unused code

4f84002

Create .gitignore to prevent unnecessary files from being copied

2c5f79b

zonuexe force-pushed the refactor/phpactor-update branch from 4dce93b to 2c5f79b Compare February 8, 2019 14:41

zonuexe merged commit d180594 into develop Feb 8, 2019

zonuexe deleted the refactor/phpactor-update branch February 8, 2019 14:42

kermorgant reviewed Feb 8, 2019

View reviewed changes

Refactor phpactor-update #86

Refactor phpactor-update #86

Uh oh!

Conversation

zonuexe commented Jan 7, 2019 • edited by kermorgant Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

kermorgant left a comment

Choose a reason for hiding this comment

Uh oh!

kermorgant Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

kermorgant Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

kermorgant Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

kermorgant Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kermorgant Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

kermorgant Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

kermorgant commented Jan 20, 2019

Uh oh!

zonuexe commented Feb 5, 2019

Uh oh!

zonuexe commented Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zonuexe commented Feb 8, 2019

Uh oh!

kermorgant commented Feb 8, 2019

Uh oh!

zonuexe commented Feb 8, 2019

Uh oh!

kermorgant commented Feb 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zonuexe commented Feb 8, 2019

Uh oh!

kermorgant Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

kermorgant Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

zonuexe Feb 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zonuexe commented Feb 8, 2019

Why not composer install in the same directory as Lisp?

Uh oh!

kermorgant commented Feb 9, 2019

Uh oh!

zonuexe commented Feb 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zonuexe commented Jan 7, 2019 •

edited by kermorgant

Loading

kermorgant Jan 7, 2019 •

edited

Loading

zonuexe commented Feb 5, 2019 •

edited

Loading

kermorgant commented Feb 8, 2019 •

edited

Loading

zonuexe Feb 8, 2019 •

edited

Loading