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 auto-start support #106

Closed
wants to merge 1 commit into from
Closed

Conversation

seblemaguer
Copy link

@seblemaguer seblemaguer commented Dec 3, 2017

I am currently working on adding the auto-start support.

  • add the logic
  • Add the documentation
  • Add the test

This aims to solve #38

@seblemaguer
Copy link
Author

I took the time (sorry for that, I had problem to understand ERT) but now it should be ready to be merged if you want :)

@tosmi tosmi mentioned this pull request Nov 1, 2018
@Fuco1 Fuco1 self-requested a review November 1, 2018 11:15
@Fuco1
Copy link
Collaborator

Fuco1 commented Nov 1, 2018

Hey, looks pretty OK. Some of the formatting could use some love :)

I'm not sure how I feel about the explicit call to the auto start method.

I think the idea was to have it auto-start immeiately after prodigy-define-service is finished. However this might cause quite some trouble in case there are dependencies (which we don't have yet).

What should happen if I call the auto start function and then define another auto-start process during my session? We should at least explain the expected behaviour.

@Fuco1
Copy link
Collaborator

Fuco1 commented Nov 1, 2018

Maybe we could use after-init-hook to call that function automatically?

@seblemaguer
Copy link
Author

I have a look at this (about the formatting too :D) and I update the pull request

@seblemaguer
Copy link
Author

Hello,

I had a little bit of time (lately) to rethink about it:

  • for the formatting, I actually don't really understand what is wrong. If you can tell me, that would be great as I can learn :)
  • for the method I added it in order to have a better control. The use after-init-hook seems a good idea for me.
  • for the testing, yes. I will see to add that.

prodigy.el Show resolved Hide resolved
(prodigy-start-service service
(lambda ()
(prodigy-stop-service service nil done))))))
(with-sandbox
Copy link
Collaborator

Choose a reason for hiding this comment

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

ert-deftest-async is a macro defined in some helper file. You need to load that file before reindenting as that will also load the macro's indent rule. This is most certainly not correct.

@seblemaguer seblemaguer force-pushed the auto-start branch 3 times, most recently from 6d28856 to 0d07429 Compare March 5, 2019 17:45
@seblemaguer
Copy link
Author

I refactor some stuff:

  • the indentation based on your comments (thanks by the way I was not away of the fact that you needed to load the helper file to get the indentation rules for the macro)
  • I also took into account your comment about the function. Now it doesn't depends if we activate the auto-start before or after. I didn't add the after-init-hook documentation but I can do that.
  • I rebase everything to be in 1 commit and to take into account the evolution of the master branch.

@Fuco1
Copy link
Collaborator

Fuco1 commented Mar 5, 2019

@seblemaguer Thanks, looks good! I'll try to review this ASAP but please if I forget don't be afraid of being annoying and just ask for review! That helps me a lot to keep track of everything.

@seblemaguer
Copy link
Author

@Fuco1 can we see to finish this pr? Else I am going to loose track of it for months again :/

@rejeep
Copy link
Owner

rejeep commented Mar 13, 2019

Please don't have inline comments, better describing them with a test. Comments get out of date.

@seblemaguer
Copy link
Author

@rejeep I removed the comments

@seblemaguer
Copy link
Author

Hello,

is there any news for this?

@DamienCassou
Copy link
Collaborator

I'm closing all old PRs. If you still care, please rebase your branch and reopen the PR.

@seblemaguer
Copy link
Author

With context.el this pr became obsolete. Sorry I forgot to close it

@seblemaguer seblemaguer deleted the auto-start branch July 14, 2022 07:03
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.

4 participants