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

Tidyups #37

Merged
merged 30 commits into from
Aug 21, 2023
Merged

Tidyups #37

merged 30 commits into from
Aug 21, 2023

Conversation

purcell
Copy link
Contributor

@purcell purcell commented Aug 11, 2023

Hi Sibi, I was trying out justl but found it didn't work for me because I use envrc.el with per-project tooling and my just isn't normally installed globally. Ultimately I expect I'd need to use inheritenv to make sure the justl.el commands pick up the right local environment. I started digging into the code and found myself tweaking a few things, but still haven't fixed the root problem. In the meantime, here are a bunch of little tweaks and improvements, in separate commits so you follow the reasoning. (I've tested this briefly by installing just globally.)

@psibi
Copy link
Owner

psibi commented Aug 11, 2023

Thanks @purcell! I still haven't gone through it, but can you check why the CI is failing ?

@purcell purcell force-pushed the master branch 2 times, most recently from 0cee9a8 to 1832c18 Compare August 11, 2023 12:00
@purcell
Copy link
Contributor Author

purcell commented Aug 11, 2023

Yup, done, tests work locally for me now.

@purcell purcell force-pushed the master branch 2 times, most recently from 894567a to fcc0b82 Compare August 11, 2023 12:10
@psibi
Copy link
Owner

psibi commented Aug 11, 2023

Thanks for the PR! This looks great.

I started digging into the code and found myself tweaking a few things, but still haven't fixed the root problem.

Have you fixed the root problem ? If no, do you want to it as part of a new PR or use the same one ? (The only thing that's remaining from my end is that I need to do the testing of TRAMP integration manually)

@purcell
Copy link
Contributor Author

purcell commented Aug 11, 2023

I'd be happy to submit a subsequent PR. If you're happy with this one, go ahead and merge. I have a couple more refactorings in mind, but might not get to them soon. And I've worked around the root problem for now by installing globally, though I still hope to resolve it in due course.

@purcell purcell force-pushed the master branch 2 times, most recently from 206e918 to 8b12ec7 Compare August 12, 2023 07:56
@purcell
Copy link
Contributor Author

purcell commented Aug 12, 2023

I fixed the root issue, and also switched things to use justl --dump --dump-format=json to make parsing info about recipes easier.

@purcell
Copy link
Contributor Author

purcell commented Aug 12, 2023

There's info in the json about the parameters too, so more code around justl-exec-recipe could be tidied up in this way too. Done for today, though.

@purcell purcell force-pushed the master branch 2 times, most recently from 4ab87d6 to cd133a7 Compare August 12, 2023 10:05
@purcell
Copy link
Contributor Author

purcell commented Aug 12, 2023

(Tests broken again with latest changes, will resolve and confirm when done.)

@psibi
Copy link
Owner

psibi commented Aug 18, 2023

I tested it more and I believe some more fixes are required:

  • In the just buffer, the recipes comes in an unordered fashion. The earlier behavior was to match the same order as in the actual justfile. Probably you can use something like this to figure out the order: just --summary --unsorted
  • When I try to execute a recipe which has arguments, the completion given by it is not correct. Right now it tries to fill with some elisp code. But the behavior should be this: If a default argument is specified in justfile, that should be populated. If not, nothing should be populated. (You can also check the behavior from the current master branch)
  • When I try to execute M-x justl-exec-recipe-in-dir, it fails with this error message:
helm-M-x-execute-command: Wrong type argument: listp

@purcell
Copy link
Contributor Author

purcell commented Aug 18, 2023

tested it more and I believe some more fixes are required:

* In the just buffer, the recipes comes in an unordered fashion. The earlier behavior was to match the same order as in the actual justfile. Probably you can use something like this to figure out the order: `just --summary --unsorted`

Retaining the original order is not supported by the --dump command, but the resulting list is sorted alphabetically (both by the elisp and – apparently – by --dump), which I think is preferable anyway. Note that the interface of just itself defaults to alphabetical, for both just --list and just --summary. The --unsorted flag has no effect on --dump.

* When I try to execute a recipe which has arguments, the completion given by it is not correct. Right now it tries to fill with some elisp code. But the behavior should be this: If a default argument is specified in justfile, that should be populated. If not, nothing should be populated. (You can also check the behavior from the current master branch)

Fixed now, but I'd note that the existing code probably isn't ideal anyway. Consider:

baz := "hello"
test foo bar=baz:
   ...

Here you can't really insert the default value such that the user can just hit return. What you'd probably really want would be to show the default in the prompt, e.g. Just arg for 'foo' [default: blah]: and then hit enter to select the default, but pass nothing through to the just invocation, ie. actually pass fewer arguments.

* When I try to execute `M-x justl-exec-recipe-in-dir`, it fails with this error message:

Fixed.

@psibi
Copy link
Owner

psibi commented Aug 18, 2023

Retaining the original order is not supported by the --dump command, but the resulting list is sorted alphabetically (both by the elisp and – apparently – by --dump)

Yeah, I know. That's why I suggested the --summary --unsorted option to get the recipe names separately and preserve ordering.

which I think is preferable anyway.

Unfortunately, my use case requires it. To give you an explicit example, I have this justfile which I use often use during testing: https://github.com/psibi/app_k8s/blob/master/justfile
The different targets there has same pattern: build, apply and remove. If they don't come in the same order as specified in the justfile, I would have to keep hunting from the huge number of similar recipes present.

The --unsorted flag has no effect on --dump.

Yeah, that's unfortunate. You have to do another call to just --summary --unsorted to figure out the proper order in the current setup.

Fixed now, but I'd note that the existing code probably isn't ideal anyway.

Yeah, agree. With your changes, I think implementing something like that would be easier in future since we can resolve variables using the json dump that we are getting.

Fixed

Thank you! My elisp has become better as I have learnt good amount of things in this PR.

@purcell
Copy link
Contributor Author

purcell commented Aug 18, 2023

almost there, just working around what appears to be an emacs bug

@purcell
Copy link
Contributor Author

purcell commented Aug 19, 2023

Alrighty, that's done. 😅

@psibi
Copy link
Owner

psibi commented Aug 19, 2023

@purcell Just tested and this looks great. I believe there is just one more change: When I try to execute a recipe which has an argument, it prompts like this:

Just arg for `version':

Can it be instead like this:

Just arg for 'version':

The only difference is that in the second version there is single quote. Also, can you update the changelog too (Probably add a unreleased section) ?

@purcell
Copy link
Contributor Author

purcell commented Aug 19, 2023

That's a pretty standard style for printing these things in Emacs, so I typed that backquote deliberately, but equally happy to change it. Probably be tomorrow. :D

@purcell
Copy link
Contributor Author

purcell commented Aug 19, 2023

Done now

@psibi psibi merged commit 30350de into psibi:master Aug 21, 2023
@psibi
Copy link
Owner

psibi commented Aug 21, 2023

Thank you!

@purcell
Copy link
Contributor Author

purcell commented Aug 21, 2023

🤝

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