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

[WIP] Refactor the definitions of RPC functions #63

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kermorgant
Copy link
Contributor

Thanks to @ejmr for this pull request (he's the author of this proposal)


IMPORTANT: This code is unfinished and needs a lot more testing.
Developers should not build off this commit because it is highly
likely we will rebase it once we address all problems.

We define a number of functions whose sole purpose is to invoke
Phpactor's RPC commands. The majority of these functions are the same.
That is, their only real difference is the name of the RPC command and
what we pass to our phpactor--rpc function. This patch refactors
that code for multiple purposes.

  1. We now only need to change one place to add support for a new
    command: the 'phpactor--rpc-command-data' list. The exception to
    this are the few RPC commands like "echo" which require additional
    arguments. It seemed easier to leave those few functions as they
    are instead of trying to shoe-horn support for a corner case into
    our now macro.

  2. The new 'phpactor-defrpc' macro automatically creates the functions
    we previously wrote by hand. This both reduces bloat and decreases
    chances for errors, since now our code for such functions is all in
    one place: the macro definition itself.

  3. We create a list of relevant data about the RPC commands which we
    can loop over, apply our new macro to each. This lets us introduce
    support for a new RPC command by modifying a single line in a list
    as opposed to writing an entire function. (Again, the exception are
    those few commands like "echo" which require additional arguments.)

----------------------------------------------------------------
IMPORTANT: This code is unfinished and needs a lot more testing.
Developers should not build off this commit because it is highly
likely we will rebase it once we address all problems.
----------------------------------------------------------------

We define a number of functions whose sole purpose is to invoke
Phpactor's RPC commands.  The majority of these functions are the same.
That is, their only real difference is the name of the RPC command and
what we pass to our `phpactor--rpc` function.  This patch refactors
that code for multiple purposes.

1. We now only need to change one place to add support for a new
command: the 'phpactor--rpc-command-data' list.  The exception to
this are the few RPC commands like "echo" which require additional
arguments.  It seemed easier to leave those few functions as they
are instead of trying to shoe-horn support for a corner case into
our now macro.

2. The new 'phpactor-defrpc' macro automatically creates the functions
we previously wrote by hand.  This both reduces bloat and decreases
chances for errors, since now our code for such functions is all in
one place: the macro definition itself.

3. We create a list of relevant data about the RPC commands which we
can loop over, apply our new macro to each.  This lets us introduce
support for a new RPC command by modifying a single line in a list
as opposed to writing an entire function.  (Again, the exception are
those few commands like "echo" which require additional arguments.)
@kermorgant
Copy link
Contributor Author

I was going to give this another try but this time, it starts giving this type of error

phpactor.el:541:1:Error: Wrong type argument: stringp, (car rpc-data)

@ejmr
Copy link

ejmr commented Sep 22, 2018

I get the same issue. Maybe instead of a macro we replace it with a function-defining-function and then (mapcar) over all the relevant data in the defconst list. Just brainstorming aloud.

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.

2 participants